-
Notifications
You must be signed in to change notification settings - Fork 42
AI-Dynamo updates. #791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
AI-Dynamo updates. #791
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors AI Dynamo workload models and public API (GenAIPerf, LMCache, LMBench, WorkerConfig), adds percentile and OpenAI TTFT tooling and wrapper scripts, updates SLURM/Kubernetes execution (container paths, pod wrapper), rewrites ai_dynamo.sh, and adds/updates tests and toml configs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/test_acceptance.py (1)
469-479: Missing requirednamefield causes pipeline failure.
GenAIPerfinherits fromBenchmarkwhich requires anamefield. The pipeline error confirms:
GenAIPerf: field 'name' is required. ValidationError: 1 validation error for GenAIPerf; name field is missing.Either provide
namehere, or fix theGenAIPerfclass to have a default (preferred fix at the class level).🐛 Workaround if class-level fix isn't applied
genai_perf=GenAIPerf( + name="genai_perf", **{ "streaming": True,tests/report_generation_strategy/test_ai_dynamo_report_generation_strategy.py (2)
89-121: Test expects report generation behavior that was removed.This test calls
strategy.generate_report()and then asserts thatreport.csvexists with specific content. However,generate_report()is now implemented aspass(no-op), so:
report_file.is_file()assertion on line 94 will fail- All subsequent content assertions are unreachable
Either the test should be removed/updated to match the new no-op behavior, or
generate_report()needs a proper implementation.
124-147: Metric tests expect different file naming and metric key formats.The test fixture creates
profile_genai_perf.csv, butget_metric()now looks forgenai_perf-report.csv. Additionally, the tests use kebab-case metric names like"output-token-throughput", but the implementation searches for exact matches in the CSV's"Metric"column (e.g.,"Output Token Throughput (tokens/sec)").These tests will return
METRIC_ERRORinstead of the expected values.tests/slurm_command_gen_strategy/test_ai_dynamo_slurm_command_gen_strategy.py (1)
60-74: Missing requirednamefield forGenAIPerf.The pipeline failure indicates:
GenAIPerf: missing required field 'name' in definitionThis instantiation needs the required
namefield.🐛 Workaround
genai_perf=GenAIPerf( + name="genai_perf", **{ "endpoint-type": "chat",src/cloudai/systems/kubernetes/kubernetes_system.py (1)
379-389: Use list-based subprocess call for cp_logs_cmd.Similar to the earlier commands, avoid
shell=Truefor the log copy operation.♻️ Suggested fix
- cp_logs_cmd = " ".join( - [ - "kubectl", - "cp", - f"{self.default_namespace}/{frontend_pod}:{genai_perf_results_path}", - str(job.test_run.output_path / "genai-perf"), - ] - ) + cp_logs_cmd = [ + "kubectl", + "cp", + f"{self.default_namespace}/{frontend_pod}:{genai_perf_results_path}", + str(job.test_run.output_path / "genai-perf"), + ] logging.debug(f"Copying genai-perf results with command: {cp_logs_cmd}") - p = subprocess.run(cp_logs_cmd, shell=True, capture_output=True, text=True) + p = subprocess.run(cp_logs_cmd, capture_output=True, text=True)
🤖 Fix all issues with AI agents
In `@src/cloudai/systems/kubernetes/kubernetes_system.py`:
- Around line 376-378: The current except block for lazy.k8s.client.ApiException
only logs the error and then allows execution to continue (risking copying
missing results); update the except to stop further processing by either
re-raising the caught exception (raise) or returning early from the enclosing
function/method so downstream code that copies results does not run; modify the
except block that catches lazy.k8s.client.ApiException (the one referencing
frontend_pod) to either raise the same exception (e) or return an appropriate
failure value immediately.
- Around line 311-316: The kubectl copy calls use subprocess.run with shell=True
and f-strings (wrapper_script_path, calc_csv_script_path, frontend_pod,
pod_wrapper_path, pod_calc_csv_path) which risks shell injection; change both
subprocess.run invocations in the method to pass the command as a list of
arguments (e.g. ["kubectl", "cp", wrapper_script_path,
f"{self.default_namespace}/{frontend_pod}:{pod_wrapper_path}"]) and remove
shell=True so Python executes the command directly; do the same for the calc CSV
call (use ["kubectl","cp", calc_csv_script_path,
f"{self.default_namespace}/{frontend_pod}:{pod_calc_csv_path}"]) while keeping
capture_output=True, text=True, and check=True.
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py`:
- Around line 41-59: The Benchmark base model declares a required field name:
str but subclasses GenAIPerf and LMBench don't provide defaults, causing
validation errors when instantiated without args; update the GenAIPerf and
LMBench classes to declare a default name (e.g., name: str = "GenAIPerf" and
name: str = "LMBench" or other canonical defaults) so Pydantic validation
succeeds, ensuring any logic using Benchmark.report_name continues to reference
the subclass name.
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh`:
- Around line 864-895: The issue is that the variable CUFILE_ENV_PATH_JSON on
line 867 is not quoted, which can cause word splitting if the path contains
spaces. To fix this, quote the variable in the redirection operator when writing
to the file in the setup_cufile function, changing the line to use
"$CUFILE_ENV_PATH_JSON" instead of $CUFILE_ENV_PATH_JSON to ensure correct
handling of paths with spaces.
- Around line 852-862: The command line in the genai-perf wrapper should quote
all variable expansions to avoid word-splitting for paths/args: wrap
${genai_perf_wrapper_script}, $RESULTS_DIR, ${genai_perf_report_file},
${calc_percentile_csv_script}, $(_gpus_per_node), the
dynamo_args/genai_perf_argument/genai_perf_args expansions, the redirection
target ${RESULTS_DIR}/genai_perf.log, and the touch "$DONE_MARKER" in quotes;
update the invocation in the block that builds the genai-perf command
(references: genai_perf_wrapper_script, RESULTS_DIR, genai_perf_report_file,
calc_percentile_csv_script, _gpus_per_node, dynamo_args["genai-perf-cmd"],
genai_perf_arguments, genai_perf_args["--extra-args"], DONE_MARKER) so every
variable is used as "${VAR}" (or the appropriate quoted form for
associative/array elements) and the log file path is quoted too.
- Around line 993-1018: In launch_lmbench, guard the pushd and quote expansions:
check pushd "$RESULTS_DIR" and if it fails log an error and exit or touch
"$DONE_MARKER" and return; after a successful pushd ensure you popd on exit;
quote all variable expansions like "${lmbench_dir}", "${RESULTS_DIR}",
"${log_file}", "${calc_percentile_csv_script}", and use proper quoting when
invoking the command and args (e.g. "$cmd" "${lmbench_arguments}"
"${lmbench_args["--extra-args"]}" or convert cmd/args into an array) to avoid
word-splitting and globbing and ensure safe handling of spaces in
launch_lmbench.
- Around line 898-914: The setup_kvbm function currently does an unsafe rm -rf
on DYN_KVBM_DISK_CACHE_DIR; before removing, validate and normalize the path
(use realpath) and ensure it is non-root, absolute, does not contain globs, and
is within an allowed base directory (e.g., a configured CACHE_BASE like
/var/cache or /tmp) or matches a safe whitelist; if validation fails, log an
error and exit instead of deleting. Also ensure all uses of
${DYN_KVBM_DISK_CACHE_DIR} are quoted to avoid word-splitting, and only perform
rm -rf after these checks succeed; keep the call to setup_cufile unchanged.
- Around line 561-563: The install step for "uv" currently pipes curl to sh and
blindly sources $HOME/.local/bin/env; change it to download the installer to a
temp file (using curl -fL), verify integrity (checksum or signature) before
executing, run the installer explicitly and check its exit status, and only
source $HOME/.local/bin/env after confirming the file exists and the install
succeeded; on any failure log an error via the existing log function and exit
with non-zero status. If possible prefer a package manager installation path
(apt/yum) or document verification requirements instead of curl | sh.
- Around line 1031-1047: The nvidia-smi invocation in function
log_gpu_utilization builds the output filename without quoting environment
variables, so paths containing spaces break; update the nvidia-smi -f argument
to quote the constructed path using the RESULTS_DIR and SLURM_NODEID variables
(e.g., use "${RESULTS_DIR}/gpu_utilization-${SLURM_NODEID}.csv") so the shell
treats it as a single filename when calling nvidia-smi in log_gpu_utilization.
- Around line 789-797: The launch_lmcache_controller function currently invokes
${dynamo_args["lmcache-controller-cmd"]} unquoted which can cause word-splitting
and broken argument parsing; update the call in launch_lmcache_controller to run
the command safely by quoting the variable or invoking it via bash -c (e.g.,
pass "${dynamo_args["lmcache-controller-cmd"]}" to bash -c or use eval-safe
invocation) so that dynamo_args["lmcache-controller-cmd"] with spaces/args is
executed correctly and output still redirected to
${RESULTS_DIR}/lmcache_controller.log.
- Around line 174-183: The min function calls use unquoted parameter expansions
which can cause word-splitting; update the two invocations that set
genai_perf_args["--warmup-request-count"] and genai_perf_args["--request-count"]
to quote their arguments (reference the genai_perf_args array keys
"--warmup-request-count" and "--request-count" and the dynamo_args keys
"min-warmup-request-count" and "min-request-count") so the calls become min
"${genai_perf_args[... ]}" "${dynamo_args[... ]}" ensuring safe expansion.
- Around line 959-991: In function launch_single_shot: remove or use the
computed but unused variable max_ctx_tokens_following (derived from isl and
cache-hit-pct) — either pass it into the python call as
--max_ctx_tokens_following or delete the computation; add robust error handling
around pushd/popd by checking their return status (or replace with a safe cd and
trap to ensure popd happens) so the script exits on directory change failure;
and ensure all variable expansions are quoted (e.g., $lmcache_path,
$RESULTS_DIR, $DONE_MARKER, $url, ${dynamo_args[…]} and any command
substitutions) to avoid word-splitting and globbing issues.
- Around line 1061-1063: The script unconditionally does cd $RESULTS_DIR before
launching log_gpu_utilization, which can fail silently; update the ai_dynamo.sh
around the cd to verify the directory exists and the cd succeeds (e.g., test for
existence or create it, then attempt cd and on failure log an error and exit
non‑zero) before starting log_gpu_utilization in the background so the logging
function isn't started in the wrong working directory; reference the RESULTS_DIR
variable, the cd invocation, and the log_gpu_utilization function when making
the change.
- Around line 799-811: The clear_lmcache function is defined but never used;
either remove it or call it during LMCache setup/benchmark startup to ensure a
clean cache state — for example invoke clear_lmcache from the LMCache
initialization path (e.g., inside start_lmcache, setup_lmcache, or before
run_benchmarks) making sure lmcache_config is available and handle curl
failures/log non-zero exits, or delete the clear_lmcache function to avoid dead
code if you don't want an automatic cache clear.
- Around line 916-957: In setup_lmcache, add a safety check before any
destructive rm -rf to validate storage_cachedir (e.g., ensure it is non-empty,
not "/" or ".", resolve with realpath and verify it is under an expected base
like $RESULTS_DIR or contains the frontend node) and abort the removal if the
check fails; also quote all variable expansions to prevent word-splitting and
globbing — specifically quote $lmcache_path in the uv pip install call, quote
"$storage_cachedir" in rm -rf and mkdir -p, quote "$LMCACHE_CONFIG_FILE" for
export/rm/-redirects, and quote "$key", "$val" and "$nkey" in the echo redirects
when writing the config; update function setup_lmcache and references to
lmcache_path, storage_cachedir, LMCACHE_CONFIG_FILE, and lmcache_config
accordingly.
- Around line 815-820: The prefill paths are intentionally disabled here:
replace the hardcoded want_prefill=0 and have_prefill=0 (which currently comment
out calls to _expected_ready_prefill and _count_initialized_prefill) with an
explanatory inline comment stating why prefill workers are disabled (e.g.,
"prefill disabled for single-node testing/temporarily pending refactor —
decode-only mode") or, if not intentional, re-enable/refactor by restoring the
real calls to _expected_ready_prefill and _count_initialized_prefill and
removing unused prefill code; reference the variables want_prefill/have_prefill
and the helper functions _expected_ready_prefill/_count_initialized_prefill and
the commented prefill launch to locate the relevant code to update.
- Around line 416-419: The appended replacement value can be split if not
properly quoted; update the append that builds result so the expanded
replacement from replace_placeholders (the val variable derived from arr[$key]
in the loop) is quoted when concatenating—use the existing result concatenation
that includes ${key} but ensure the replacement is inserted as "${val}"
(preserving spaces/special chars) so modify the code around result+="${key}
${val} " to quote the val expansion.
In `@src/cloudai/workloads/ai_dynamo/calc_percentile_csv.py`:
- Around line 61-65: The parse_float_safe function currently catches a broad
Exception; narrow it to only the expected float conversion failures by catching
ValueError and TypeError instead (i.e., change the except Exception to except
(ValueError, TypeError)) and keep the same behavior of returning float("nan") so
parse_float_safe(value: Any) still returns NaN for invalid inputs.
In `@src/cloudai/workloads/ai_dynamo/genai_perf_wrapper.sh`:
- Line 60: The assignment to profile_path uses an unquoted $result_dir which can
cause globbing and word-splitting for paths with spaces; update the find
invocation (the profile_path assignment) to quote $result_dir (and any other
shell expansions in that command) so the path is treated as a single argument
and safe from splitting/globbing.
- Around line 47-52: The code flattens the genai_perf_cmdline_args array into a
single string losing argument boundaries and then tries to execute that string;
instead keep it as an array and execute it via the array expansion. Replace the
assignment genai_perf_cmdline_args="${genai_perf_cmdline_args[*]}" (remove it),
change the log to safely show arguments (for example use printf '%q '
"${genai_perf_cmdline_args[@]}" or join with spaces for display), and launch the
command using the array expansion "${genai_perf_cmdline_args[@]}" so arguments
with spaces or special characters are preserved when executing.
- Around line 62-66: The script currently overwrites the percentile report
because the grep line (`grep ".*,.*,.*,.*" $profile_path >
$result_dir/$report_file`) uses a single >; change it to append (>>) so the
earlier python3 `${calc_percentile_csv_script} $profile_path -o
$result_dir/$report_file` output is preserved. Replace the integer division for
`output_tokens_per_second_per_gpu=$(( $output_tokens_per_second / $total_gpus
))` with a floating-point calculation (e.g., using `bc` or `awk`, or
`output_tokens_per_second_per_gpu=$(awk -v a="$output_tokens_per_second" -v
b="$total_gpus" 'BEGIN{printf "%.6f", a/b}')`) to avoid truncation, and keep the
final echo that appends the value to `$result_dir/$report_file`.
- Around line 41-43: In the case statement of genai_perf_wrapper.sh the default
branch "*)" currently silences unknown arguments; update that branch (the "*)"
handler) to report unrecognized options by printing an error to stderr
(including the offending "$1" value) and either show brief usage/help or suggest
correct flags, then exit non‑zero (e.g., exit 1) so typos or unsupported args
are caught instead of ignored.
In `@src/cloudai/workloads/ai_dynamo/report_generation_strategy.py`:
- Around line 69-70: The generate_report method is currently a no-op and must be
implemented to satisfy test_ai_dynamo_generate_report: update the
ReportGenerationStrategy.generate_report implementation to create a report.csv
file (in the same output/location the tests expect) and write the expected CSV
headers and rows the test asserts against; use the class's available data
members or helper methods to collect the rows, open the file in write mode, and
write CSV-formatted content (including correct headers and values) so the test
can find the file and validate its contents.
- Around line 27-28: Remove the empty TYPE_CHECKING block because it contains
only a no-op pass; delete the lines containing "if TYPE_CHECKING:" and the
following "pass" in report_generation_strategy.py so there is no dead code
related to TYPE_CHECKING in the module.
- Line 23: Replace the direct pandas import with the project's lazy import
utility to avoid startup overhead: remove "import pandas as pd" and instead
reference LazyImports.pd (from src.cloudai.util.lazy_imports) wherever pandas is
used in ReportGenerationStrategy or this module; ensure you import the
LazyImports symbol (e.g., "from src.cloudai.util.lazy_imports import
LazyImports") and update any code that expects a module object to call
LazyImports.pd instead so pandas is loaded lazily only when needed.
In `@src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py`:
- Around line 242-244: The log may report the wrong "from" value because
self.test_run.num_nodes is modified earlier; capture the original value into a
local (e.g., original_num_nodes = self.test_run.num_nodes) before any
assignments and use that in the logging.info call (log "Setting num_nodes from
%d to %d", original_num_nodes, total_nodes) right before you set
self.test_run.num_nodes = total_nodes in the method that contains the current
code (referencing self.test_run.num_nodes and total_nodes in
slurm_command_gen_strategy.py).
In `@tests/json_gen_strategy/test_ai_dynamo.py`:
- Line 49: The test instantiates GenAIPerf without the required Benchmark field
"name", causing validation to fail; update the GenAIPerf construction in
tests/json_gen_strategy/test_ai_dynamo.py (the genai_perf=GenAIPerf(...) site)
to pass a string name (e.g., a short descriptive identifier) so the Benchmark
validation succeeds and CI stops failing.
In
`@tests/report_generation_strategy/test_ai_dynamo_report_generation_strategy.py`:
- Line 67: The GenAIPerf instantiation is missing the required Benchmark 'name'
argument; update the test where GenAIPerf() is constructed (the GenAIPerf
symbol) to pass a name string (e.g., GenAIPerf(name="genai_perf")) so the
required field from the Benchmark parent is provided and the test can construct
the object successfully.
In `@tests/test_calc_percentile_csv.py`:
- Line 18: The import statement "from pathlib import Path" in the test file is
unused; remove that import line so the test module no longer contains the unused
symbol Path and the CI lint error is resolved—locate the import at the top of
tests/test_calc_percentile_csv.py (the "from pathlib import Path" line) and
delete it.
| def generate_report(self) -> None: | ||
| output_path = self.test_run.output_path | ||
| source_csv = next(output_path.rglob(CSV_FILES_PATTERN)) | ||
| target_csv = output_path / "report.csv" | ||
|
|
||
| total_gpus = self._calculate_total_gpus() | ||
| if total_gpus is None: | ||
| logging.warning("gpus_per_node is None, skipping Overall Output Tokens per Second per GPU calculation.") | ||
| shutil.copy2(source_csv, target_csv) | ||
| return | ||
|
|
||
| sections = self._read_csv_sections(source_csv) | ||
| self._write_sections_with_metric(target_csv, sections, total_gpus) | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generate_report is now a no-op, breaking test assertions.
The test_ai_dynamo_generate_report test expects generate_report() to produce a report.csv file with specific content, but this method now does nothing. Either the test needs updating or the implementation needs to be completed.
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/report_generation_strategy.py` around lines
69 - 70, The generate_report method is currently a no-op and must be implemented
to satisfy test_ai_dynamo_generate_report: update the
ReportGenerationStrategy.generate_report implementation to create a report.csv
file (in the same output/location the tests expect) and write the expected CSV
headers and rows the test asserts against; use the class's available data
members or helper methods to collect the rows, open the file in write mode, and
write CSV-formatted content (including correct headers and values) so the test
can find the file and validate its contents.
tests/report_generation_strategy/test_ai_dynamo_report_generation_strategy.py
Show resolved
Hide resolved
9ad787e to
c4617f7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cloudai/workloads/ai_dynamo/ai_dynamo.sh (1)
1072-1074: Add error handling for cd to workspace path.Line 1073 changes to
${dynamo_args["workspace-path"]}without checking if the operation succeeds or quoting the path. This could silently fail if the directory doesn't exist.♻️ Proposed fix
if _is_vllm; then - cd ${dynamo_args["workspace-path"]} + cd "${dynamo_args["workspace-path"]}" || { + log "ERROR: Failed to change to workspace directory: ${dynamo_args["workspace-path"]}" + exit 1 + } fi
🤖 Fix all issues with AI agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py`:
- Around line 74-76: The serializer _serialize_repo currently returns the
misleading literal "/invalid/repo/path" when repo is None; change it to return
None (or empty string if your schema requires) instead to avoid masking
configuration errors: replace the else branch with "None" (i.e. return
v.container_mount if v else None), update the return type to Optional[str] if
necessary, and adjust any callers/consumers that expect a string to handle None
(or validate/raise an explicit error inside _serialize_repo if you prefer
failing-fast).
- Around line 183-187: The GitRepo instances are using SSH-style URLs which
break CI without SSH keys; update the GitRepo constructor calls (e.g., the
variable repo of type Optional[GitRepo] and the other GitRepo usage around the
second occurrence) to use HTTPS-formatted repository URLs (e.g.,
https://github.com/<org>/<repo>.git) instead of "git@github.com:..."; ensure
both places where GitRepo(...) is invoked are changed so CI/CD environments can
clone without SSH auth.
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh`:
- Around line 448-476: Separate the declaration and assignment to address SC2155
in exit_on_error: instead of combining local and command substitution for fatal,
first declare the variable (local fatal) and then assign it with
fatal=$(_detect_fatal_once); locate this in the exit_on_error function where
fatal is set (currently using _detect_fatal_once) and make the two-step change
so shellcheck warning SC2155 is resolved.
- Around line 210-211: The case branch for genai_perf args has a prefix
mismatch: the pattern uses `--genai_perf-args-*` but the extraction uses
`${key#--genai-perf-args-}`, so `${key#...}` won't strip the actual prefix and
will leave a leftover in the index; update them to be consistent (either change
the case pattern to `--genai-perf-args-*` or change the extraction to
`${key#--genai_perf-args-}`) in the branch handling `genai_perf_args` (look for
the `genai_perf_args["${key#...}"]="$2"` line and the `--genai_perf-args-*`
case) so the array keys are computed correctly.
- Around line 550-560: The workload-type checks (_is_genai_perf_workload,
_is_lmbench_workload, _is_single_shot_workload) rely on
dynamo_args["workload-type"] which may be unset; initialize a sensible default
for dynamo_args["workload-type"] (e.g., "genai-perf" or empty string) at
dynamo_args population/initialization point so those predicate functions behave
predictably when CLI input is absent, and ensure any existing code that sets
dynamo_args still allows the default to be overridden.
In `@src/cloudai/workloads/ai_dynamo/calc_percentile_csv.py`:
- Around line 106-110: The current logic silently does nothing for completely
empty CSVs (no headers and no rows); detect this case early by checking rows
and/or the DictReader fieldnames and emit a user-friendly warning or logger
message before returning/continuing; update the code around the variables rows
and all_columns in calc_percentile_csv.py to log a clear warning (e.g., "empty
CSV: no headers or data") when both rows is empty and there are no fieldnames,
and then skip the numeric-summary loop that calls parse_float_safe and
append_summary so behavior is explicit and debuggable.
In `@src/cloudai/workloads/ai_dynamo/report_generation_strategy.py`:
- Around line 37-47: The extract_metric_from_csv function assumes a "Metric"
column exists but only checks metric_type; update extract_metric_from_csv to
first validate that "Metric" is in df.columns (in addition to metric_type), and
if missing log a clear message including df.columns and return METRIC_ERROR;
then proceed to check metric_name in df["Metric"].values and return the metric
as before. Ensure you reference the existing symbols metric_type, metric_name,
METRIC_ERROR and use the same logging style as the other checks.
- Around line 55-56: The current metric parsing in report_generation_strategy.py
unpacks metric.split(":") into three variables which raises ValueError for
inputs with not exactly three parts; modify the parsing in the block handling
the variable metric (the code that currently does `benchmark_name, metric_name,
metric_type = metric.split(":")`) to safely split with at most two splits (e.g.
split(":", 2)) and then normalize missing parts with sensible defaults (e.g. set
metric_type to a default like "unknown" or None when absent) or validate and
reject malformed metrics with a clear error/log message; ensure the change is
applied wherever the metric variable is processed so downstream code that
expects benchmark_name, metric_name, and metric_type receives well-defined
values.
In `@src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py`:
- Around line 193-194: The assert messages incorrectly reference
dynamo.num_prefill_nodes and dynamo.num_decode_nodes; update the assertions in
slurm_command_gen_strategy.py that validate prefill_n and decode_n so the error
messages reflect the actual attributes (e.g., "prefill_worker.num_nodes must be
an integer" and "decode_worker.num_nodes must be an integer") or use the
variable names ("prefill_n must be an integer", "decode_n must be an integer")
for clarity; change the assert statements that check isinstance(prefill_n, int)
and isinstance(decode_n, int) to use one of these corrected messages.
In `@tests/ref_data/ai-dynamo.sbatch`:
- Line 31: Remove the duplicate `--export=ALL` flag from the sbatch command in
the script so only one `--export=ALL` remains; locate the second occurrence of
`--export=ALL` (the duplicate entry added near the sbatch options) and delete
it, leaving the original instance intact.
- Line 69: The generated SLURM argument construction in
slurm_command_gen_strategy.py (the list comprehension building args from
toml_args using prefix and the f-string at the comprehension) wraps values with
double quotes which breaks JSON-containing values; change the construction to
either wrap values in single quotes when the value contains double quotes or to
escape inner double quotes (e.g., detect if v contains '"' then use
f"{prefix}{k} '{v}'" or escape quotes) so that JSON like {"temperature": 0.7,
"max_tokens": 128} is emitted as a single shell-safe token (use the same change
where args is built and any helper that formats values).
In
`@tests/report_generation_strategy/test_ai_dynamo_report_generation_strategy.py`:
- Around line 117-119: The test empties the wrong CSV due to a filename
mismatch: replace the hyphenated filename with the underscore variant so the
existing test CSV is cleared; specifically, update the write_text call that
currently targets (ai_dynamo_tr.output_path / "genai_perf-report.csv") to target
"genai_perf_report.csv" so the subsequent call to
strategy.get_metric("invalid-metric") exercises the empty-file error path
instead of creating a new file.
- Around line 86-91: The test currently only asserts True; instead validate a
concrete behavior of AIDynamoReportGenerationStrategy.generate_report: call
strategy.generate_report(), capture its return value and assert it is None (or
the expected sentinel), and/or assert no report file was created by checking the
expected report path derived from the strategy (use
AIDynamoReportGenerationStrategy.report_path or the strategy instance to
determine the output file) or check a relevant side-effect on the ai_dynamo_tr
fixture; update test_ai_dynamo_generate_report to replace the tautological
assert with one or two assertions verifying the method's return and absence (or
presence if expected) of the report file or expected state change.
| @field_serializer("repo") | ||
| def _serialize_repo(self, v: GitRepo) -> str: | ||
| return v.container_mount if v else "/invalid/repo/path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serializer returns misleading path for None repo.
When repo is None, the serializer returns "/invalid/repo/path" which could mask configuration issues downstream. Consider returning None or an empty string, or raising an error during serialization if the repo is unexpectedly missing.
🐛 Proposed fix
`@field_serializer`("repo")
- def _serialize_repo(self, v: GitRepo) -> str:
- return v.container_mount if v else "/invalid/repo/path"
+ def _serialize_repo(self, v: Optional[GitRepo]) -> Optional[str]:
+ return v.container_mount if v else None🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py` around lines 74 - 76, The
serializer _serialize_repo currently returns the misleading literal
"/invalid/repo/path" when repo is None; change it to return None (or empty
string if your schema requires) instead to avoid masking configuration errors:
replace the else branch with "None" (i.e. return v.container_mount if v else
None), update the return type to Optional[str] if necessary, and adjust any
callers/consumers that expect a string to handle None (or validate/raise an
explicit error inside _serialize_repo if you prefer failing-fast).
| repo: Optional[GitRepo] = GitRepo( | ||
| url="git@github.com:LMCache/LMCache.git", | ||
| commit="ab8530993992db873869ba882320953582d94309", | ||
| mount_as="/git/LMCache", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
rg -n "GitRepo\(" --type=py -A3 | grep -E "(url=|https://|git@)"Repository: NVIDIA/cloudai
Length of output: 1971
Use HTTPS URLs for GitRepo to improve CI/CD compatibility.
The SSH URLs at lines 184 and 214 should be changed to HTTPS format (https://github.com:...) to ensure compatibility in environments without SSH key authentication. This aligns with HTTPS URLs used elsewhere in the codebase (e.g., nemo_launcher.py, megatron_bridge.py, and in ai_dynamo.py at line 255).
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py` around lines 183 - 187, The
GitRepo instances are using SSH-style URLs which break CI without SSH keys;
update the GitRepo constructor calls (e.g., the variable repo of type
Optional[GitRepo] and the other GitRepo usage around the second occurrence) to
use HTTPS-formatted repository URLs (e.g., https://github.com/<org>/<repo>.git)
instead of "git@github.com:..."; ensure both places where GitRepo(...) is
invoked are changed so CI/CD environments can clone without SSH auth.
| function perform_exit() | ||
| { | ||
| local exit_code=$1 | ||
| local sleep_before_exit="${dynamo_args["sleep-before-exit"]}" | ||
| if [[ -n "${sleep_before_exit}" ]]; then | ||
| log "Sleeping for ${sleep_before_exit} seconds before exit" | ||
| sleep "${sleep_before_exit}" | ||
| fi | ||
| exit "${exit_code}" | ||
| } | ||
|
|
||
| exit_on_error() { | ||
| local fatal=$(_detect_fatal_once) | ||
| if [ -f "${DONE_MARKER}" ]; then | ||
| log "DONE_MARKER found. Skipping error check." | ||
| return | ||
| fi | ||
| if [[ "${fatal}" -gt 0 ]]; then | ||
| log "FATAL: detected ${fatal} fatal error line(s). Writing ${FATAL_ERROR_MARKER} and terminating." | ||
| sleep 1 | ||
|
|
||
| touch "${FATAL_ERROR_MARKER}" | ||
| grep -E "${dynamo_args["worker-error-pattern"]}|UCX.*ERROR" "${RESULTS_DIR}"/*.log 2>/dev/null > "${FATAL_ERROR_MARKER}" | ||
| # Try to stop background jobs for a cleaner exit, but do not loop | ||
| kill $(jobs -p) 2>/dev/null || true | ||
| # Exit non-zero so srun can retry | ||
| exit 1 | ||
| perform_exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
LGTM with minor suggestion.
The error handling improvements are good:
perform_exitprovides configurable exit delay for debugging- Checking
DONE_MARKERbefore error detection prevents false positives after successful runs
Minor: Line 460 could separate declaration and assignment per SC2155, but this is stylistic.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 460-460: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 472-472: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 448 - 476,
Separate the declaration and assignment to address SC2155 in exit_on_error:
instead of combining local and command substitution for fatal, first declare the
variable (local fatal) and then assign it with fatal=$(_detect_fatal_once);
locate this in the exit_on_error function where fatal is set (currently using
_detect_fatal_once) and make the two-step change so shellcheck warning SC2155 is
resolved.
| assert isinstance(prefill_n, int), "dynamo.num_prefill_nodes must be an integer" | ||
| assert isinstance(decode_n, int), "dynamo.num_decode_nodes must be an integer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertion messages reference non-existent attribute names.
The messages say dynamo.num_prefill_nodes and dynamo.num_decode_nodes, but the actual attributes being checked are prefill_worker.num_nodes and decode_worker.num_nodes.
🐛 Proposed fix
- assert isinstance(prefill_n, int), "dynamo.num_prefill_nodes must be an integer"
- assert isinstance(decode_n, int), "dynamo.num_decode_nodes must be an integer"
+ assert isinstance(prefill_n, int), "prefill_worker.num_nodes must be an integer"
+ assert isinstance(decode_n, int), "decode_worker.num_nodes must be an integer"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert isinstance(prefill_n, int), "dynamo.num_prefill_nodes must be an integer" | |
| assert isinstance(decode_n, int), "dynamo.num_decode_nodes must be an integer" | |
| assert isinstance(prefill_n, int), "prefill_worker.num_nodes must be an integer" | |
| assert isinstance(decode_n, int), "decode_worker.num_nodes must be an integer" |
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py` around lines
193 - 194, The assert messages incorrectly reference dynamo.num_prefill_nodes
and dynamo.num_decode_nodes; update the assertions in
slurm_command_gen_strategy.py that validate prefill_n and decode_n so the error
messages reflect the actual attributes (e.g., "prefill_worker.num_nodes must be
an integer" and "decode_worker.num_nodes must be an integer") or use the
variable names ("prefill_n must be an integer", "decode_n must be an integer")
for clarity; change the assert statements that check isinstance(prefill_n, int)
and isinstance(decode_n, int) to use one of these corrected messages.
| --ntasks=2 \ | ||
| --ntasks-per-node=1 \ | ||
| --export=ALL,DYNAMO_FATAL_ERROR_FILE=fatal_error.marker \ | ||
| --export=ALL \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Duplicate --export=ALL flag.
Line 31 duplicates --export=ALL which is already specified on line 23. While not harmful, it adds unnecessary redundancy to the command.
♻️ Suggested fix
--nodes=2 \
--ntasks=2 \
--ntasks-per-node=1 \
- --export=ALL \
--output=__OUTPUT_DIR__/output/node-%n-stdout.txt \🤖 Prompt for AI Agents
In `@tests/ref_data/ai-dynamo.sbatch` at line 31, Remove the duplicate
`--export=ALL` flag from the sbatch command in the script so only one
`--export=ALL` remains; locate the second occurrence of `--export=ALL` (the
duplicate entry added near the sbatch options) and delete it, leaving the
original instance intact.
| def test_ai_dynamo_generate_report(slurm_system: SlurmSystem, ai_dynamo_tr: TestRun, csv_content: str) -> None: | ||
| strategy = AIDynamoReportGenerationStrategy(slurm_system, ai_dynamo_tr) | ||
| # The new implementation does not generate a report file | ||
| strategy.generate_report() | ||
|
|
||
| report_file = ai_dynamo_tr.output_path / "report.csv" | ||
| assert report_file.is_file(), "Report CSV was not generated." | ||
|
|
||
| report_content = report_file.read_text() | ||
|
|
||
| def split_into_sections(content: str) -> list[str]: | ||
| sections = content.split("\n\n") | ||
| return [s.strip() for s in sections if s.strip()] | ||
|
|
||
| def normalize_csv_section(section: str) -> str: | ||
| return section.replace('"', "").strip() | ||
|
|
||
| actual_sections = [normalize_csv_section(s) for s in split_into_sections(report_content)] | ||
| expected_sections = [normalize_csv_section(s) for s in split_into_sections(csv_content)] | ||
|
|
||
| # First section should match after normalization | ||
| assert actual_sections[0] == expected_sections[0], "First section (metrics) does not match" | ||
|
|
||
| # Second section should have our additional metric | ||
| second_section_lines = actual_sections[1].split("\n") | ||
| assert second_section_lines[0] == "Metric,Value", "Second section header does not match" | ||
| assert second_section_lines[1] == "Output Token Throughput (tokens/sec),24", "Throughput line does not match" | ||
| assert second_section_lines[2] == "Overall Output Tokens per Second per GPU,1.0", "Added metric line is incorrect" | ||
| assert second_section_lines[3:] == ["Request Throughput (per sec),1.23", "Request Count (count),40.00"], ( | ||
| "Remaining lines do not match" | ||
| ) | ||
|
|
||
| # Third section (GPU metrics) should be identical | ||
| assert actual_sections[2] == expected_sections[2], "Third section (GPU metrics) does not match" | ||
| # Just verify the method runs without error | ||
| assert True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider adding a meaningful assertion.
The test only verifies that generate_report() runs without error. An assert True is a tautology. Consider asserting on a specific behavior or removing the redundant assertion.
♻️ Suggested improvement
def test_ai_dynamo_generate_report(slurm_system: SlurmSystem, ai_dynamo_tr: TestRun, csv_content: str) -> None:
strategy = AIDynamoReportGenerationStrategy(slurm_system, ai_dynamo_tr)
# The new implementation does not generate a report file
- strategy.generate_report()
- # Just verify the method runs without error
- assert True
+ # Verify the method runs without raising an exception
+ strategy.generate_report() # No assertion needed - no exception = success📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_ai_dynamo_generate_report(slurm_system: SlurmSystem, ai_dynamo_tr: TestRun, csv_content: str) -> None: | |
| strategy = AIDynamoReportGenerationStrategy(slurm_system, ai_dynamo_tr) | |
| # The new implementation does not generate a report file | |
| strategy.generate_report() | |
| report_file = ai_dynamo_tr.output_path / "report.csv" | |
| assert report_file.is_file(), "Report CSV was not generated." | |
| report_content = report_file.read_text() | |
| def split_into_sections(content: str) -> list[str]: | |
| sections = content.split("\n\n") | |
| return [s.strip() for s in sections if s.strip()] | |
| def normalize_csv_section(section: str) -> str: | |
| return section.replace('"', "").strip() | |
| actual_sections = [normalize_csv_section(s) for s in split_into_sections(report_content)] | |
| expected_sections = [normalize_csv_section(s) for s in split_into_sections(csv_content)] | |
| # First section should match after normalization | |
| assert actual_sections[0] == expected_sections[0], "First section (metrics) does not match" | |
| # Second section should have our additional metric | |
| second_section_lines = actual_sections[1].split("\n") | |
| assert second_section_lines[0] == "Metric,Value", "Second section header does not match" | |
| assert second_section_lines[1] == "Output Token Throughput (tokens/sec),24", "Throughput line does not match" | |
| assert second_section_lines[2] == "Overall Output Tokens per Second per GPU,1.0", "Added metric line is incorrect" | |
| assert second_section_lines[3:] == ["Request Throughput (per sec),1.23", "Request Count (count),40.00"], ( | |
| "Remaining lines do not match" | |
| ) | |
| # Third section (GPU metrics) should be identical | |
| assert actual_sections[2] == expected_sections[2], "Third section (GPU metrics) does not match" | |
| # Just verify the method runs without error | |
| assert True | |
| def test_ai_dynamo_generate_report(slurm_system: SlurmSystem, ai_dynamo_tr: TestRun, csv_content: str) -> None: | |
| strategy = AIDynamoReportGenerationStrategy(slurm_system, ai_dynamo_tr) | |
| # The new implementation does not generate a report file | |
| # Verify the method runs without raising an exception | |
| strategy.generate_report() # No assertion needed - no exception = success |
🤖 Prompt for AI Agents
In
`@tests/report_generation_strategy/test_ai_dynamo_report_generation_strategy.py`
around lines 86 - 91, The test currently only asserts True; instead validate a
concrete behavior of AIDynamoReportGenerationStrategy.generate_report: call
strategy.generate_report(), capture its return value and assert it is None (or
the expected sentinel), and/or assert no report file was created by checking the
expected report path derived from the strategy (use
AIDynamoReportGenerationStrategy.report_path or the strategy instance to
determine the output file) or check a relevant side-effect on the ai_dynamo_tr
fixture; update test_ai_dynamo_generate_report to replace the tautological
assert with one or two assertions verifying the method's return and absence (or
presence if expected) of the report file or expected state change.
| # Empty the CSV file to test error handling | ||
| (ai_dynamo_tr.output_path / "genai_perf-report.csv").write_text("") | ||
| assert strategy.get_metric("invalid-metric") == METRIC_ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filename mismatch: genai_perf-report.csv vs genai_perf_report.csv.
Line 68 creates genai_perf_report.csv (underscore), but line 118 writes to genai_perf-report.csv (hyphen). This test writes to a non-existent file (creating a new empty one) rather than clearing the existing test data file. The subsequent assertion passes but doesn't test the intended error-handling path for an empty CSV.
🐛 Proposed fix
# Empty the CSV file to test error handling
- (ai_dynamo_tr.output_path / "genai_perf-report.csv").write_text("")
+ (ai_dynamo_tr.output_path / "genai_perf_report.csv").write_text("")
assert strategy.get_metric("invalid-metric") == METRIC_ERROR🤖 Prompt for AI Agents
In
`@tests/report_generation_strategy/test_ai_dynamo_report_generation_strategy.py`
around lines 117 - 119, The test empties the wrong CSV due to a filename
mismatch: replace the hyphenated filename with the underscore variant so the
existing test CSV is cleared; specifically, update the write_text call that
currently targets (ai_dynamo_tr.output_path / "genai_perf-report.csv") to target
"genai_perf_report.csv" so the subsequent call to
strategy.get_metric("invalid-metric") exercises the empty-file error path
instead of creating a new file.
14a051b to
e433300
Compare
Signed-off-by: Kapil Arya <kapila@nvidia.com>
- Remove unused imports (field_serializer, GitRepo, File) - Fix code formatting (ruff format) - Fix TOML indentation (taplo fmt) - Fix pyright type error in _get_nested_toml_args - Fix GitRepo dict check to not require installed_path - Update test reference file for LMBench field changes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix clear_kv_cache to use /clear_kv_blocks endpoint (not /reset_prefix_cache) - Add explicit port fields to AIDynamoArgs (port, etcd_port, nats_port) - Pass connector and kvbm_metrics_port from ai_dynamo.sh to workload scripts - Update custom_workload.sh to receive all port config via command line args - Only fetch KVBM metrics when connector is kvbm and port is specified Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Greptile OverviewGreptile SummaryThis PR updates the experimental AI-Dynamo workload integration and its scenarios/tests to match newer AI-Dynamo use cases. The bulk of the change refreshes workload scripts/configs (vLLM scenarios across Slurm/Kubernetes, wrapper scripts like Main merge blockers are concentrated in workload execution and reporting: several changes (many already discussed in prior threads) can cause runtime failures (missing required args, unsafe/incorrect command/arg serialization, disabled report generation). In the latest commit, Confidence Score: 2/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 files reviewed, 2 comments
| prefill_tp = prefill_worker.args.tensor_parallel_size if prefill_worker else 1 | ||
| decode_tp = decode_worker.args.tensor_parallel_size if decode_worker else 1 | ||
| prefill_pp = prefill_worker.args.pipeline_parallel_size if prefill_worker else 1 | ||
| decode_pp = decode_worker.args.pipeline_parallel_size if decode_worker else 1 | ||
| prefill_nodes = prefill_worker.num_nodes if prefill_worker else 0 | ||
| decode_nodes = decode_worker.num_nodes if decode_worker else 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accessing .args.tensor_parallel_size without checking if .args exists first
| prefill_tp = prefill_worker.args.tensor_parallel_size if prefill_worker else 1 | |
| decode_tp = decode_worker.args.tensor_parallel_size if decode_worker else 1 | |
| prefill_pp = prefill_worker.args.pipeline_parallel_size if prefill_worker else 1 | |
| decode_pp = decode_worker.args.pipeline_parallel_size if decode_worker else 1 | |
| prefill_nodes = prefill_worker.num_nodes if prefill_worker else 0 | |
| decode_nodes = decode_worker.num_nodes if decode_worker else 1 | |
| prefill_tp = prefill_worker.args.tensor_parallel_size if prefill_worker and prefill_worker.args else 1 | |
| decode_tp = decode_worker.args.tensor_parallel_size if decode_worker and decode_worker.args else 1 | |
| prefill_pp = prefill_worker.args.pipeline_parallel_size if prefill_worker and prefill_worker.args else 1 | |
| decode_pp = decode_worker.args.pipeline_parallel_size if decode_worker and decode_worker.args else 1 |
| prefill_config["gpus-per-worker"]=$(( prefill_args["--tensor-parallel-size"] * prefill_args["--pipeline-parallel-size"] )) | ||
| decode_config["gpus-per-worker"]=$(( decode_args["--tensor-parallel-size"] * decode_args["--pipeline-parallel-size"] )) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arithmetic will fail if --tensor-parallel-size or --pipeline-parallel-size are unset (empty string results in invalid arithmetic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/cloudai/workloads/ai_dynamo/ai_dynamo.sh (4)
149-193: 🧹 Nitpick | 🔵 TrivialInconsistent key prefixing across associative arrays.
lmcache_argskeys have no--prefix (line 166) whilelmbench_args,genai_perf_args, andcustom_workload_argsdo (lines 170, 174, 178). Similarly,lmcache_configkeys lack--whilelmbench_config,genai_perf_config,custom_workload_configinclude it.This is presumably intentional (lmcache values are accessed by key directly, others go through
array_to_args), but the inconsistency is a maintenance trap. Consider adding a brief comment near the declarations (lines 30-41) explaining which arrays use---prefixed keys and why.
836-856:⚠️ Potential issue | 🟠 MajorPrefill readiness check is bypassed while prefill workers are still launched.
Lines 838 and 842 hardcode
want_prefill=0andhave_prefill=0, bypassing prefill readiness validation. However,launch_prefillis still called at line 1079 when_is_prefill_node. This meanswait_for_dynamo_frontend(line 1023) will proceed to launch workloads before prefill workers have initialized, potentially causing failures.Either re-enable the prefill readiness checks or add a comment explaining why prefill readiness is intentionally skipped (e.g., prefill workers initialize fast enough or workloads don't depend on prefill).
419-435:⚠️ Potential issue | 🟠 Major
shopt -s nocasematchleaks into global shell state.
nocasematchis enabled on line 424 inside the loop but never reverted. Afterarray_to_argsreturns, all subsequent[[ ... == ... ]]comparisons in the script become case-insensitive, which could cause subtle match failures (e.g., backend detection, connector checks).🐛 Proposed fix: save and restore shopt state
function array_to_args() { local -n arr=$1 local result="" + local _prev_nocasematch + _prev_nocasematch=$(shopt -p nocasematch 2>/dev/null || true) for key in "${!arr[@]}"; do shopt -s nocasematch val=$(replace_placeholders "${arr[$key]}") # Quote values that contain spaces if [[ "$val" == *" "* ]]; then val="${val//\"/\\\"}" # Escape existing quotes result+="${key} \"${val}\" " else result+="${key} ${val} " fi done + eval "$_prev_nocasematch" echo "$result" }The same issue exists in
arg_array_to_string(line 367) — thoughnocasematchisn't set there currently, it's worth noting.
746-823: 🧹 Nitpick | 🔵 TrivialLGTM with minor notes.
Worker launch logic is correct. The connector injection and port offset calculations handle multi-worker-per-node scenarios properly.
Minor:
$log_fileon lines 779 and 822 should be quoted to handle paths with spaces (> "$log_file" 2>&1).
🤖 Fix all issues with AI agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py`:
- Around line 337-352: The installables list in the installables property
currently has self.hf_model commented out without explanation; either re-enable
self.hf_model in the returned list (remove the comment) if HF model downloads
should be part of installables, or leave it commented and add a short clarifying
comment next to the commented line explaining why HF model is excluded (e.g.,
"HF model download is performed elsewhere" or "temporarily disabled—tracked by
ISSUE-XXXX"); if you choose to keep it disabled, also create or link an issue to
track re-enabling and reference that issue ID in the comment.
- Around line 46-48: Update the docstring for the Workload class to accurately
describe its purpose (e.g., "Base configuration for a workload" or "Represents
the base configuration and metadata for a custom workload") instead of the
copy-pasted "Arguments for custom workloads"; locate the Workload class
definition inheriting from BaseModel and replace the misleading docstring with a
concise, accurate description that reflects the class role and contents.
- Around line 158-164: The prefill_worker field currently defaults to a
WorkerConfig instance which makes truthiness checks (e.g., in
get_cached_nodes_spec, constraint_check, kubernetes_json_gen_strategy) behave as
if prefill is configured; change prefill_worker to be optional by updating its
annotation to Optional[WorkerConfig] and set Field(default=None) (remove
default_factory), so unconfigured manifests produce None; update any
serialization/validation aliases if present and run tests that exercise
get_cached_nodes_spec, constraint_check, and kubernetes_json_gen_strategy to
confirm behavior.
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh`:
- Around line 858-874: The _query_frontend function is dead code and builds JSON
via fragile string interpolation using dynamo_args["model"] and content, which
can break for special characters; either remove the unused _query_frontend
function or (if keeping for debugging) rewrite it to construct the payload
securely with jq (already required earlier) and then POST the safe JSON file at
"$RESULTS_DIR/curl_cmd.json" to "${dynamo_args["url"]}/v1/chat/completions",
ensuring proper escaping of dynamo_args["model"], the messages array, and
max_tokens instead of naive string concatenation.
- Around line 938-940: The log call references lmcache_path before it's
assigned, so move the assignment local lmcache_path="${lmcache_config["repo"]}"
above the log or change the log to reference the repo directly; update the block
around the log and the uv pip install -e $lmcache_path call (both referencing
lmcache_path) so the variable is assigned prior to use (ensure
lmcache_config["repo"] exists) and the install uses the same lmcache_path
variable.
- Around line 988-1019: In launch_workload remove the unused nameref
workload_args_ref (the array_to_args call already takes the name string) to
avoid dead code, and update the invocation of the workload script and log
redirect to safely handle spaces by quoting the path expressions: quote
"${INSTALL_DIR}/$script" where the script is executed and quote the log file
path as "${RESULTS_DIR}/$workload_name.log"; leave array_to_args usage unchanged
and keep other dynamo_args references as-is.
- Around line 196-221: The function _populate_nodelist currently uses cut -d','
-f1-${num_nodes} which only examines the first num_nodes entries and can miss
valid nodes when some of those are excluded; change the logic in
_populate_nodelist to iterate over all entries in DYNAMO_NODELIST (split on
commas) and collect nodes not present in exclude_nodelist until you have
num_nodes, then stop; keep existing behavior of returning empty string for
zero/empty num_nodes, trimming the trailing comma from nodelist before echo, and
continue using the same identifiers (num_nodes, exclude_nodelist,
DYNAMO_NODELIST, _populate_nodelist) to locate and update the code.
- Around line 909-916: The setup_storage_cache_dir function mutates the global
STORAGE_CACHE_DIR by appending to it, making it non-idempotent and causing
nested paths on repeated calls; change it to compute the final path in a local
variable (e.g., local target_dir) using TEST_USER, dynamo_args["frontend-node"],
and dynamo_args["connector"], perform rm -rf, mkdir -p and chmod 755 on that
local variable, then assign the global STORAGE_CACHE_DIR only if it is unset (or
explicitly intended), and ensure all uses of the path in rm/mkdir/chmod are
quoted to avoid word splitting.
In `@src/cloudai/workloads/ai_dynamo/custom_workload.sh`:
- Around line 148-162: In function print_metrics, remove or restore use of the
unused local variables frontend_metrics_endpoint and component_metrics_endpoint
to address ShellCheck SC2034; either delete the lines that set
frontend_metrics_endpoint and component_metrics_endpoint if you intend only to
log KVBM metrics, or uncomment and re-enable the corresponding curl+log blocks
that reference those variables (the commented curl/grep and log lines) so
frontend_metrics_endpoint and component_metrics_endpoint are actually used.
- Around line 247-260: The bytes_per_token computed by the awk expression can be
zero which will cause a division-by-zero when computing g2_token_size; after
calculating bytes_per_token (from g1_memory_total_gib and g1_token_size) add a
guard that checks if bytes_per_token is zero or unset and if so log an error via
log and skip or set g2_token_size to 0 instead of performing the division;
ensure the branch that uses DYN_KVBM_CPU_CACHE_GB only performs
g2_token_size=$(( (g2_cache_gb * 1024 * 1024 * 1024) / bytes_per_token )) when
bytes_per_token is non-zero (use the same log function and variables
g1_memory_total_gib, g1_token_size, bytes_per_token, g2_cache_gb to locate the
code).
In `@src/cloudai/workloads/ai_dynamo/openai_chat_client.py`:
- Around line 68-73: Replace the unexplained magic number 37 used to compute
num_tokens by dynamically measuring the tokenizer overhead of the chat prompt
template instead of hardcoding it: build the chat/template string used around
the user doc (or a placeholder like "<DOC>"), call self.tok.encode on that
template (add_special_tokens=False, truncation=True) to get overhead =
len(template_ids) - len(placeholder_ids) or simply len(template_ids) and then
compute num_tokens = self.isl - overhead; update code around raw_doc,
num_tokens, and tok.encode to use this computed overhead and add a short comment
explaining the measurement so the behavior adapts to different models/tokenizers
(replace all uses of the literal 37).
- Line 46: The log print in openai_chat_client currently contains an extra space
inside the f-string before the closing brace in time.strftime (the expression
"time.strftime('%Y-%m-%d %H:%M:%S') }"), which adds a trailing space to every
timestamp; remove that space so the f-string ends "...{time.strftime('%Y-%m-%d
%H:%M:%S')}] ..." and keep the rest of the print (including
os.getenv('HOSTNAME') and the message variable) unchanged.
- Around line 244-249: Rename the duplicate "Run 1" warmup label to avoid
confusion: change the log call that currently prints "=== Run 1: warmup ==="
(the warmup before calling Chat(...).stream) to a distinct label such as "===
Warmup ===" or "=== Run 0: warmup ===", so the subsequent log "=== Run 1:
baseline TTFT ===" remains accurate; update only the string passed to log around
the warmup variable (warmup = Chat(args.model, args.isl).stream(client, 1) and
its follow-up log) to reflect the new label.
- Around line 184-185: The minFillerPrompts assignment uses min(1,
args.min_filler_prompts) which clamps values to at most 1 and breaks logic;
change it to max(1, args.min_filler_prompts) so minFillerPrompts enforces a
floor of 1 while preserving larger user inputs; update the line where
maxFillerPrompts and minFillerPrompts are set (references: maxFillerPrompts,
minFillerPrompts, args.max_filler_prompts, args.min_filler_prompts).
In `@src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py`:
- Around line 112-119: The if-check guarding prefill args is ineffective because
AIDynamoArgs.prefill_worker uses default_factory=WorkerConfig and is always
truthy; either remove the redundant if and always call
self._get_nested_toml_args(td.cmd_args.dynamo.prefill_worker, "--prefill-"), or
change the condition to a real sentinel (e.g., check
td.cmd_args.dynamo.prefill_worker.num_nodes > 0 or an explicit enabled flag on
WorkerConfig) so that prefill args are only added when prefill is actually
configured; update the call site in slurm_command_gen_strategy where
td.cmd_args.dynamo.prefill_worker is referenced and ensure the chosen sentinel
exists on WorkerConfig.
- Around line 34-35: Replace f-string interpolation inside logging calls with
lazy %-style formatting: change logging.info(f"hf_home_path:
{td.cmd_args.hf_home_path}") and logging.info(f"storage_cache_dir:
{td.cmd_args.storage_cache_dir}") in slurm_command_gen_strategy.py to use
logging.info("hf_home_path: %s", td.cmd_args.hf_home_path) and
logging.info("storage_cache_dir: %s", td.cmd_args.storage_cache_dir"). Do the
same pattern for the other occurrences referenced in ai_dynamo.py (lines around
the logging calls in ai_dynamo functions) so all logging uses logger.info("msg
%s", value) instead of f-strings to preserve lazy formatting.
| class Workload(BaseModel): | ||
| """Arguments for custom workloads.""" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Docstring is copy-pasted from Args.
The Workload class docstring says "Arguments for custom workloads" — should describe what Workload actually represents (e.g., "Base configuration for a workload").
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py` around lines 46 - 48, Update
the docstring for the Workload class to accurately describe its purpose (e.g.,
"Base configuration for a workload" or "Represents the base configuration and
metadata for a custom workload") instead of the copy-pasted "Arguments for
custom workloads"; locate the Workload class definition inheriting from
BaseModel and replace the misleading docstring with a concise, accurate
description that reflects the class role and contents.
| decode_worker: WorkerConfig = Field(default_factory=WorkerConfig) | ||
| decode_cmd: str = Field( | ||
| default="python3 -m dynamo.vllm", | ||
| serialization_alias="decode-cmd", | ||
| validation_alias=AliasChoices("decode-cmd", "decode_cmd"), | ||
| ) | ||
| prefill_worker: PrefillWorkerArgs | None = None | ||
| prefill_worker: WorkerConfig = Field(default_factory=WorkerConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the prefill_worker definition at line 158-164
sed -n '158,164p' src/cloudai/workloads/ai_dynamo/ai_dynamo.pyRepository: NVIDIA/cloudai
Length of output: 384
🏁 Script executed:
# Find the full AIDynamoArgs class definition to see prefill_worker context
sed -n '130,170p' src/cloudai/workloads/ai_dynamo/ai_dynamo.pyRepository: NVIDIA/cloudai
Length of output: 1476
🏁 Script executed:
# Check the WorkerConfig definition to understand num_nodes default
rg -n "class WorkerConfig" src/cloudai/workloads/ai_dynamo/ -A 15Repository: NVIDIA/cloudai
Length of output: 1423
🏁 Script executed:
# Examine the get_cached_nodes_spec method in slurm_command_gen_strategy.py
rg -n "def get_cached_nodes_spec" src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py -A 25Repository: NVIDIA/cloudai
Length of output: 1257
🏁 Script executed:
# Check how the if-condition on line 184 is supposed to work - look for any explicit None assignments
rg -n "prefill_worker.*None" src/cloudai/workloads/ai_dynamo/Repository: NVIDIA/cloudai
Length of output: 40
🏁 Script executed:
# Check test configurations to see how prefill_worker is set
find src/cloudai/workloads/ai_dynamo -name "*.py" | xargs rg -l "prefill_worker" | head -5Repository: NVIDIA/cloudai
Length of output: 228
🏁 Script executed:
# Look at a test or example config to see expected behavior
rg -n "prefill_worker" src/cloudai/workloads/ai_dynamo/ -B1 -A1 | head -40Repository: NVIDIA/cloudai
Length of output: 3717
🏁 Script executed:
# Check if there's any logic that explicitly sets prefill_worker to None or checks for presence before use
rg -n "prefill_worker" src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py -B2 -A2Repository: NVIDIA/cloudai
Length of output: 957
🏁 Script executed:
# Check if there are any test configuration files that show how prefill_worker should be used
find . -name "*.yaml" -o -name "*.yml" -o -name "*.toml" | xargs rg -l "prefill" 2>/dev/null | head -5Repository: NVIDIA/cloudai
Length of output: 358
🏁 Script executed:
# Look at kubernetes_json_gen_strategy.py line 66 to understand the expectation
sed -n '60,70p' src/cloudai/workloads/ai_dynamo/kubernetes_json_gen_strategy.pyRepository: NVIDIA/cloudai
Length of output: 542
🏁 Script executed:
# Check if there's a conditional way that prefill_worker could be None or falsy anywhere
sed -n '360,380p' src/cloudai/workloads/ai_dynamo/ai_dynamo.pyRepository: NVIDIA/cloudai
Length of output: 1223
🏁 Script executed:
# Look at the full constraint_check method and surrounding context
sed -n '370,400p' src/cloudai/workloads/ai_dynamo/ai_dynamo.pyRepository: NVIDIA/cloudai
Length of output: 1758
🏁 Script executed:
# Look at the git diff or history to see what the previous type was
git log --oneline -n 20 src/cloudai/workloads/ai_dynamo/ai_dynamo.pyRepository: NVIDIA/cloudai
Length of output: 99
🏁 Script executed:
# Check if there's a test config that explicitly doesn't set prefill_worker to see expected behavior
cat ./conf/experimental/ai_dynamo/test/vllm.tomlRepository: NVIDIA/cloudai
Length of output: 2046
🏁 Script executed:
# Check the other test config
cat ./conf/experimental/ai_dynamo/test_scenario/vllm_slurm.tomlRepository: NVIDIA/cloudai
Length of output: 1806
🏁 Script executed:
# Verify the behavior in get_cached_nodes_spec with decode-only config
# Check lines 183-193 for the logic
sed -n '182,210p' src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.pyRepository: NVIDIA/cloudai
Length of output: 1475
🏁 Script executed:
# Double-check: what would be prefill_n if prefill_worker has default WorkerConfig(num_nodes=1)?
# Trace through: line 184 condition would always pass, line 185 assigns prefill_n = 1
# Then line 199 would calculate total_nodes as 1 (prefill) + decode_n
# For vllm.toml decode-only, this would be 1 + 1 = 2 instead of 1
sed -n '199,205p' src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.pyRepository: NVIDIA/cloudai
Length of output: 302
prefill_worker default causes decode-only setups to over-allocate nodes.
prefill_worker defaults to WorkerConfig(num_nodes=1). In get_cached_nodes_spec() (slurm_command_gen_strategy.py line 184), the check if td.cmd_args.dynamo.prefill_worker: is always true because Pydantic BaseModel instances are always truthy. This means prefill_n gets set to the default num_nodes=1 even when prefill is not configured, causing total_nodes = 1 + decode_n instead of just decode_n. For the decode-only test config (vllm.toml), this allocates 2 nodes instead of 1.
Other code expects prefill_worker to be None when unconfigured: constraint_check() at line 381 uses if prefill_worker else 0, and kubernetes_json_gen_strategy.py line 66 checks if not prefill_worker: and raises an error.
Make prefill_worker optional:
- prefill_worker: WorkerConfig = Field(default_factory=WorkerConfig)
+ prefill_worker: Optional[WorkerConfig] = None🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py` around lines 158 - 164, The
prefill_worker field currently defaults to a WorkerConfig instance which makes
truthiness checks (e.g., in get_cached_nodes_spec, constraint_check,
kubernetes_json_gen_strategy) behave as if prefill is configured; change
prefill_worker to be optional by updating its annotation to
Optional[WorkerConfig] and set Field(default=None) (remove default_factory), so
unconfigured manifests produce None; update any serialization/validation aliases
if present and run tests that exercise get_cached_nodes_spec, constraint_check,
and kubernetes_json_gen_strategy to confirm behavior.
| @property | ||
| def installables(self) -> list[Installable]: | ||
| return [self.docker_image, self.script, self.dynamo_repo, self.hf_model] | ||
| """Get all installables for this test definition.""" | ||
| result = [ | ||
| self.docker_image, | ||
| self.script, | ||
| #self.hf_model, | ||
| self.genai_perf_script, | ||
| self.calc_percentile_csv, | ||
| self.cmd_args.lmbench.script, | ||
| self.cmd_args.custom_workload.script, | ||
| File(Path(__file__).parent.parent / "ai_dynamo/openai_chat_client.py"), | ||
| *self.git_repos, | ||
| ] | ||
|
|
||
| return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Commented-out self.hf_model in installables.
Line 343 has #self.hf_model commented out. If this is intentional (e.g., HF model download handled elsewhere), add a brief comment explaining why. If it's temporary, track it.
Do you want me to open an issue to track re-enabling self.hf_model in installables?
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.py` around lines 337 - 352, The
installables list in the installables property currently has self.hf_model
commented out without explanation; either re-enable self.hf_model in the
returned list (remove the comment) if HF model downloads should be part of
installables, or leave it commented and add a short clarifying comment next to
the commented line explaining why HF model is excluded (e.g., "HF model download
is performed elsewhere" or "temporarily disabled—tracked by ISSUE-XXXX"); if you
choose to keep it disabled, also create or link an issue to track re-enabling
and reference that issue ID in the comment.
| _query_frontend() { | ||
| local content="${1:-The color of sky is}" | ||
| content=$(echo "$content" | sed 's/"/\\"/g' | sed 's/\n/\\n/g') | ||
| local max_tokens="${2:-10}" | ||
|
|
||
| local json='{ | ||
| "model": "'${dynamo_args["model"]}'", | ||
| "messages": [{"role": "user", "content": "The color of sky is"}], | ||
| "messages": [{"role": "user", "content": "'"$content"'"}], | ||
| "stream": false, | ||
| "max_tokens": 10 | ||
| "max_tokens": '$max_tokens', | ||
| "temperature": 0, | ||
| "top_p": 0.0001 | ||
| }' | ||
| curl -s -X POST "${dynamo_args["url"]}/v1/chat/completions" -H "Content-Type: application/json" -d "$json" | ||
|
|
||
| echo "$json" > "$RESULTS_DIR/curl_cmd.json" | ||
| curl -s -X POST "${dynamo_args["url"]}/v1/chat/completions" -H "Content-Type: application/json" -d @$RESULTS_DIR/curl_cmd.json | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
_query_frontend is never invoked and uses fragile JSON construction.
This function (flagged by ShellCheck SC2329) is dead code in the current flow. If retained for debugging, note that the JSON on lines 863-870 is built via string interpolation — a model name with special characters would break the JSON. Consider using jq (already required at line 643) for safe JSON construction.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 858-874: This function is never invoked. Check usage (or ignored if invoked indirectly).
(SC2329)
[info] 873-873: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 858 - 874, The
_query_frontend function is dead code and builds JSON via fragile string
interpolation using dynamo_args["model"] and content, which can break for
special characters; either remove the unused _query_frontend function or (if
keeping for debugging) rewrite it to construct the payload securely with jq
(already required earlier) and then POST the safe JSON file at
"$RESULTS_DIR/curl_cmd.json" to "${dynamo_args["url"]}/v1/chat/completions",
ensuring proper escaping of dynamo_args["model"], the messages array, and
max_tokens instead of naive string concatenation.
| raw_doc = "".join(random.choices(string.ascii_letters + string.digits, k=self.isl * 4)) | ||
|
|
||
| num_tokens = self.isl - 37 | ||
| ids = self.tok.encode(raw_doc, add_special_tokens=False, truncation=True, max_length=num_tokens) | ||
| assert len(ids) == num_tokens, f"Expected {num_tokens} tokens, got {len(ids)}" | ||
| doc = self.tok.decode(ids, skip_special_tokens=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 37 is unexplained and likely model-dependent.
Line 70 uses self.isl - 37 to account for what appears to be chat template overhead, but this number is not documented and will vary across models/tokenizers. If the model's chat template adds more or fewer tokens, the final prompt won't match the intended isl.
Consider deriving the overhead dynamically (e.g., tokenize the template with a placeholder and measure the delta), or at minimum document what 37 represents.
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/openai_chat_client.py` around lines 68 - 73,
Replace the unexplained magic number 37 used to compute num_tokens by
dynamically measuring the tokenizer overhead of the chat prompt template instead
of hardcoding it: build the chat/template string used around the user doc (or a
placeholder like "<DOC>"), call self.tok.encode on that template
(add_special_tokens=False, truncation=True) to get overhead = len(template_ids)
- len(placeholder_ids) or simply len(template_ids) and then compute num_tokens =
self.isl - overhead; update code around raw_doc, num_tokens, and tok.encode to
use this computed overhead and add a short comment explaining the measurement so
the behavior adapts to different models/tokenizers (replace all uses of the
literal 37).
| maxFillerPrompts = args.max_filler_prompts | ||
| minFillerPrompts = min(1, args.min_filler_prompts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: min(1, args.min_filler_prompts) always clamps to ≤ 1, ignoring user input.
min(1, x) returns 1 when x ≥ 1 and returns x when x < 1 (which would be 0 or negative — breaking the binary search). This should be max(1, ...) to enforce a floor of 1 while respecting larger user-supplied values.
Suggested fix
- minFillerPrompts = min(1, args.min_filler_prompts)
+ minFillerPrompts = max(1, args.min_filler_prompts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| maxFillerPrompts = args.max_filler_prompts | |
| minFillerPrompts = min(1, args.min_filler_prompts) | |
| maxFillerPrompts = args.max_filler_prompts | |
| minFillerPrompts = max(1, args.min_filler_prompts) |
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/openai_chat_client.py` around lines 184 -
185, The minFillerPrompts assignment uses min(1, args.min_filler_prompts) which
clamps values to at most 1 and breaks logic; change it to max(1,
args.min_filler_prompts) so minFillerPrompts enforces a floor of 1 while
preserving larger user inputs; update the line where maxFillerPrompts and
minFillerPrompts are set (references: maxFillerPrompts, minFillerPrompts,
args.max_filler_prompts, args.min_filler_prompts).
| log("=== Run 1: warmup ===") | ||
| warmup = Chat(args.model, args.isl).stream(client, 1) | ||
| log(f"Run 1: warmup: TTFT = {warmup.ttft_seconds:.3f}s with {warmup.cached_tokens} cached tokens") | ||
|
|
||
| # ---------------- RUN 1 ---------------- | ||
| log("=== Run 1: baseline TTFT ===") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate "Run 1" label is confusing.
Line 244 logs "=== Run 1: warmup ===" and line 249 logs "=== Run 1: baseline TTFT ===". The warmup should be labeled separately (e.g., "Run 0" or just "Warmup") to avoid ambiguity in logs.
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/openai_chat_client.py` around lines 244 -
249, Rename the duplicate "Run 1" warmup label to avoid confusion: change the
log call that currently prints "=== Run 1: warmup ===" (the warmup before
calling Chat(...).stream) to a distinct label such as "=== Warmup ===" or "===
Run 0: warmup ===", so the subsequent log "=== Run 1: baseline TTFT ===" remains
accurate; update only the string passed to log around the warmup variable
(warmup = Chat(args.model, args.isl).stream(client, 1) and its follow-up log) to
reflect the new label.
| logging.info(f"hf_home_path: {td.cmd_args.hf_home_path}") | ||
| logging.info(f"storage_cache_dir: {td.cmd_args.storage_cache_dir}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Avoid f-string formatting in logging.info calls.
Using f-strings with logging defeats lazy formatting — the string is always interpolated even when the log level suppresses the message. Use %-style placeholders instead.
- logging.info(f"hf_home_path: {td.cmd_args.hf_home_path}")
- logging.info(f"storage_cache_dir: {td.cmd_args.storage_cache_dir}")
+ logging.info("hf_home_path: %s", td.cmd_args.hf_home_path)
+ logging.info("storage_cache_dir: %s", td.cmd_args.storage_cache_dir)This pattern recurs in ai_dynamo.py as well (lines 333, 360, 365, 368, 384, etc.).
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py` around lines
34 - 35, Replace f-string interpolation inside logging calls with lazy %-style
formatting: change logging.info(f"hf_home_path: {td.cmd_args.hf_home_path}") and
logging.info(f"storage_cache_dir: {td.cmd_args.storage_cache_dir}") in
slurm_command_gen_strategy.py to use logging.info("hf_home_path: %s",
td.cmd_args.hf_home_path) and logging.info("storage_cache_dir: %s",
td.cmd_args.storage_cache_dir"). Do the same pattern for the other occurrences
referenced in ai_dynamo.py (lines around the logging calls in ai_dynamo
functions) so all logging uses logger.info("msg %s", value) instead of f-strings
to preserve lazy formatting.
| if td.cmd_args.dynamo.prefill_worker: | ||
| args.extend(self._get_toml_args(td.cmd_args.dynamo.prefill_worker, "--prefill-")) | ||
| args.extend(self._get_toml_args(td.cmd_args.dynamo.decode_worker, "--decode-")) | ||
| args.extend(self._get_toml_args(td.cmd_args.genai_perf, "--genai-perf-")) | ||
| args.extend(self._get_nested_toml_args(td.cmd_args.dynamo.prefill_worker, "--prefill-")) | ||
| args.extend(self._get_nested_toml_args(td.cmd_args.dynamo.decode_worker, "--decode-")) | ||
|
|
||
| args.extend(self._get_nested_toml_args(td.cmd_args.lmcache, "--lmcache-")) | ||
| args.extend(self._get_nested_toml_args(td.cmd_args.genai_perf, "--genai_perf-")) | ||
| args.extend(self._get_nested_toml_args(td.cmd_args.lmbench, "--lmbench-")) | ||
| args.extend(self._get_nested_toml_args(td.cmd_args.custom_workload, "--custom_workload-")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Conditional on prefill_worker is always truthy.
AIDynamoArgs.prefill_worker has default_factory=WorkerConfig, so it's never None or falsy. The if guard on line 112 will always pass. If the intent is to skip prefill args when not configured, you'd need an explicit sentinel (e.g., checking num_nodes > 0). Otherwise, remove the guard for clarity.
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.py` around lines
112 - 119, The if-check guarding prefill args is ineffective because
AIDynamoArgs.prefill_worker uses default_factory=WorkerConfig and is always
truthy; either remove the redundant if and always call
self._get_nested_toml_args(td.cmd_args.dynamo.prefill_worker, "--prefill-"), or
change the condition to a real sentinel (e.g., check
td.cmd_args.dynamo.prefill_worker.num_nodes > 0 or an explicit enabled flag on
WorkerConfig) so that prefill args are only added when prefill is actually
configured; update the call site in slurm_command_gen_strategy where
td.cmd_args.dynamo.prefill_worker is referenced and ensure the chosen sentinel
exists on WorkerConfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 files reviewed, 9 comments
| def can_handle_directory(self) -> bool: | ||
| return True | ||
|
|
||
| def generate_report(self) -> None: | ||
| output_path = self.test_run.output_path | ||
| source_csv = next(output_path.rglob(CSV_FILES_PATTERN)) | ||
| target_csv = output_path / "report.csv" | ||
|
|
||
| total_gpus = self._calculate_total_gpus() | ||
| if total_gpus is None: | ||
| logging.warning("gpus_per_node is None, skipping Overall Output Tokens per Second per GPU calculation.") | ||
| shutil.copy2(source_csv, target_csv) | ||
| return | ||
|
|
||
| sections = self._read_csv_sections(source_csv) | ||
| self._write_sections_with_metric(target_csv, sections, total_gpus) | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Report generation now disabled
can_handle_directory() always returns True and generate_report() is now a no-op. This will cause CloudAI to select this strategy for any directory and then generate no report artifacts, which breaks prior behavior and can hide missing output files. If the intent is “metrics-only” mode, this should at least preserve can_handle_directory() gating and/or write the expected report CSV.
| assert strategy.get_metric("invalid-metric") == METRIC_ERROR | ||
|
|
||
| (ai_dynamo_tr.output_path / "profile_genai_perf.csv").write_text("") | ||
| assert strategy.get_metric("default") == METRIC_ERROR | ||
| # Empty the CSV file to test error handling | ||
| (ai_dynamo_tr.output_path / "genai_perf-report.csv").write_text("") | ||
| assert strategy.get_metric("invalid-metric") == METRIC_ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test writes wrong filename
This test empties genai_perf-report.csv, but the implementation reads genai_perf_report.csv (underscore). As written, the test doesn’t exercise the empty-file path at all.
| assert strategy.get_metric("invalid-metric") == METRIC_ERROR | |
| (ai_dynamo_tr.output_path / "profile_genai_perf.csv").write_text("") | |
| assert strategy.get_metric("default") == METRIC_ERROR | |
| # Empty the CSV file to test error handling | |
| (ai_dynamo_tr.output_path / "genai_perf-report.csv").write_text("") | |
| assert strategy.get_metric("invalid-metric") == METRIC_ERROR | |
| (ai_dynamo_tr.output_path / "genai_perf_report.csv").write_text("") |
| def constraint_check(self, tr: TestRun) -> bool: | ||
| prefill_worker = tr.test.cmd_args.dynamo.prefill_worker | ||
| decode_worker = tr.test.cmd_args.dynamo.decode_worker | ||
|
|
||
| prefill_tp = prefill_worker.args.tensor_parallel_size if prefill_worker else 1 | ||
| decode_tp = decode_worker.args.tensor_parallel_size if decode_worker else 1 | ||
| prefill_pp = prefill_worker.args.pipeline_parallel_size if prefill_worker else 1 | ||
| decode_pp = decode_worker.args.pipeline_parallel_size if decode_worker else 1 | ||
| prefill_nodes = prefill_worker.num_nodes if prefill_worker else 0 | ||
| decode_nodes = decode_worker.num_nodes if decode_worker else 1 | ||
|
|
||
| if self.constraints.prefill_tp_le_decode_tp and prefill_tp > decode_tp: | ||
| logging.info("constraint_check failed for: prefill_tp_le_decode_tp") | ||
| return False | ||
| logging.info("constraint_check passed for: prefill_tp_le_decode_tp") | ||
|
|
||
| gpus_per_node = tr.test.cmd_args.gpus_per_node | ||
| if self.constraints.tp_times_pp_le_gpus_per_node and ( | ||
| prefill_tp * prefill_pp > gpus_per_node or decode_tp * decode_pp > gpus_per_node | ||
| ): | ||
| logging.info("constraint_check failed for: tp_times_pp_le_gpus_per_node") | ||
| return False | ||
| logging.info("constraint_check passed for: tp_times_pp_le_gpus_per_node") | ||
|
|
||
| num_nodes = tr.test.cmd_args.num_nodes | ||
| nodes_check = self.constraints.prefill_decode_nodes_le_total_nodes | ||
| if nodes_check and prefill_nodes + decode_nodes > num_nodes: | ||
| logging.info("constraint_check failed for: prefill_decode_nodes_le_total_nodes") | ||
| return False | ||
| logging.info("constraint_check passed for: prefill_decode_nodes_le_total_nodes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constraint check can crash
constraint_check() multiplies prefill_tp * prefill_pp and decode_tp * decode_pp, but these fields are declared as int | list[int] | None. When unset they’ll be None, and None * None will raise TypeError (and lists will also break comparisons/multiplication). This will fail for configs that omit parallelism values (common in TOML).
Also appears in: src/cloudai/workloads/ai_dynamo/ai_dynamo.py:376-391
| logging.debug(f"Copying wrapper script {wrapper_script_path} to pod {frontend_pod}") | ||
| cp_wrapper_cmd = f"kubectl cp {wrapper_script_path} {self.default_namespace}/{frontend_pod}:{pod_wrapper_path}" | ||
| subprocess.run(cp_wrapper_cmd, shell=True, capture_output=True, text=True, check=True) | ||
|
|
||
| logging.debug(f"Copying calc_percentile_csv script {calc_csv_script_path} to pod {frontend_pod}") | ||
| cp_calc_cmd = f"kubectl cp {calc_csv_script_path} {self.default_namespace}/{frontend_pod}:{pod_calc_csv_path}" | ||
| subprocess.run(cp_calc_cmd, shell=True, capture_output=True, text=True, check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shell injection via kubectl cp
_run_genai_perf() builds kubectl cp ... commands as strings and runs them with shell=True using paths derived from installed artifacts and pod/namespace names. If any of these contain shell metacharacters (spaces, ;, etc.), the command will break or execute unintended shell tokens. This is avoidable by passing an argument list to subprocess.run (no shell) or by properly escaping.
Also note: this occurs for both cp_wrapper_cmd and cp_calc_cmd.
|
|
||
| export SLURM_JOB_MASTER_NODE=$(scontrol show hostname $SLURM_JOB_NODELIST | head -n 1) | ||
|
|
||
| srun --export=ALL --mpi=pmix -N2 --container-image=nvcr.io/nvidia/ai-dynamo:24.09 --container-mounts=__OUTPUT_DIR__/output:/cloudai_run_results,__OUTPUT_DIR__/install:/cloudai_install,__OUTPUT_DIR__/output,__INSTALL_DIR__:__INSTALL_DIR__,__INSTALL_DIR__/huggingface:/root/.cache/huggingface,__CLOUDAI_DIR__/src/cloudai/workloads/ai_dynamo/ai_dynamo.sh:__CLOUDAI_DIR__/src/cloudai/workloads/ai_dynamo/ai_dynamo.sh --output=__OUTPUT_DIR__/output/mapping-stdout.txt --error=__OUTPUT_DIR__/output/mapping-stderr.txt bash -c "echo \$(date): \$(hostname):node \${SLURM_NODEID}:rank \${SLURM_PROCID}." | ||
| srun --export=ALL --mpi=pmix -N2 --container-image=nvcr.io/nvidia/ai-dynamo:24.09 --container-mounts=__OUTPUT_DIR__/output:/cloudai_run_results,__OUTPUT_DIR__/install:/cloudai_install,__OUTPUT_DIR__/output,__INSTALL_DIR__:/git/dynamo,__INSTALL_DIR__/LMCache__ab8530993992db873869ba882320953582d94309:/git/LMCache,__INSTALL_DIR__/LMBenchmark__e1406623c5e88878cf2b7fbd64fe6c47f7dcb66f:/git/LMBenchmark,__INSTALL_DIR__/huggingface:/workspace/hf_home --output=__OUTPUT_DIR__/output/mapping-stdout.txt --error=__OUTPUT_DIR__/output/mapping-stderr.txt bash -c "echo \$(date): \$(hostname):node \${SLURM_NODEID}:rank \${SLURM_PROCID}." | ||
|
|
||
| srun --export=ALL --mpi=pmix -N2 --container-image=nvcr.io/nvidia/ai-dynamo:24.09 --container-mounts=__OUTPUT_DIR__/output:/cloudai_run_results,__OUTPUT_DIR__/install:/cloudai_install,__OUTPUT_DIR__/output,__INSTALL_DIR__:__INSTALL_DIR__,__INSTALL_DIR__/huggingface:/root/.cache/huggingface,__CLOUDAI_DIR__/src/cloudai/workloads/ai_dynamo/ai_dynamo.sh:__CLOUDAI_DIR__/src/cloudai/workloads/ai_dynamo/ai_dynamo.sh --ntasks=2 --ntasks-per-node=1 --output=__OUTPUT_DIR__/output/metadata/node-%N.toml --error=__OUTPUT_DIR__/output/metadata/nodes.err bash /cloudai_install/slurm-metadata.sh | ||
| srun --export=ALL --mpi=pmix -N2 --container-image=nvcr.io/nvidia/ai-dynamo:24.09 --container-mounts=__OUTPUT_DIR__/output:/cloudai_run_results,__OUTPUT_DIR__/install:/cloudai_install,__OUTPUT_DIR__/output,__INSTALL_DIR__:/git/dynamo,__INSTALL_DIR__/LMCache__ab8530993992db873869ba882320953582d94309:/git/LMCache,__INSTALL_DIR__/LMBenchmark__e1406623c5e88878cf2b7fbd64fe6c47f7dcb66f:/git/LMBenchmark,__INSTALL_DIR__/huggingface:/workspace/hf_home --ntasks=2 --ntasks-per-node=1 --output=__OUTPUT_DIR__/output/metadata/node-%N.toml --error=__OUTPUT_DIR__/output/metadata/nodes.err bash /cloudai_install/slurm-metadata.sh | ||
|
|
||
| num_retries=${DYNAMO_NUM_RETRY_ON_FAILURE:-0} | ||
| for try in $(seq 0 $num_retries); do | ||
| echo "Try $try of $num_retries" | ||
| rm -f __OUTPUT_DIR__/output/fatal_error.marker 2>/dev/null || true | ||
| srun \ | ||
| srun \ | ||
| --export=ALL \ | ||
| --mpi=pmix \ | ||
| -N2 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golden sbatch has unsafe args
This fixture now contains unescaped JSON/dict content in an argument (e.g. --genai_perf-extra-inputs "{"temperature": 0.7, ...}"). When used as a real sbatch script, the shell will mis-parse this into multiple tokens (or syntax errors), so the golden file encodes a command line that can’t actually run as written. This likely stems from _get_toml_args() emitting Python dict reprs rather than shell-safe strings.
Additional Comments (4)
If these args are expected to be structured JSON, they need to be emitted as a shell-safe single argument (e.g., JSON string with proper escaping) rather than Python repr.
Additionally, the log line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
22 files reviewed, 5 comments
|
|
||
| def _get_nested_toml_args(self, base_model: BaseModel, prefix: str) -> List[str]: | ||
| result = self._get_toml_args(base_model, prefix, exclude=["args"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken repo/file arg quoting
_get_toml_args() emits dict-valued fields using f'... "{v}"', which stringifies the whole dict with single quotes (Python repr). That produces shell-unsafe tokens and breaks args like --*-extra-inputs or any nested config dict when used in sbatch/srun. This is the same failure mode seen in the updated golden sbatch (unescaped {"temperature": ...} argument).
Consider serializing dict/list values to a shell-safe string (e.g., JSON via json.dumps(v)), and ensure proper quoting/escaping (or avoid string concatenation and build an argv list). Also applies to other dict-like fields not covered by the url/commit/mount_as and src special-cases.
Also appears in: tests/ref_data/ai-dynamo.sbatch:~40-70 (invalid quoting for --genai_perf-extra-inputs).
|
|
||
| assert strategy.get_metric("invalid-metric") == METRIC_ERROR | ||
|
|
||
| (ai_dynamo_tr.output_path / "profile_genai_perf.csv").write_text("") | ||
| assert strategy.get_metric("default") == METRIC_ERROR | ||
| # Empty the CSV file to test error handling | ||
| (ai_dynamo_tr.output_path / "genai_perf-report.csv").write_text("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test edits wrong file
This test empties genai_perf-report.csv, but the implementation reads ${benchmark_name}_report.csv (default benchmark_name is genai_perf), i.e. genai_perf_report.csv. As written, the “empty file” case isn’t exercised and the assertion is checking invalid-metric again.
Update the test to empty genai_perf_report.csv and assert a real metric returns METRIC_ERROR when the CSV is empty.
| result_dir="$2" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses undefined variable
cmdline_args is built using ${key}, but key is never set in this script. This will expand to an empty string (or cause issues under set -u) and indicates the command line is not what’s intended.
Remove ${key} or replace it with the correct flag/value (if you intended to pass a specific option, define it explicitly from parsed args).
|
|
||
| eval $full_cmd | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eval on constructed cmd
This script runs eval $full_cmd, where $full_cmd is assembled from config/CLI values (cmd, cmdline_args, extra_args). If any of those contain shell metacharacters, the executed command can be altered (and even benign inputs with quotes/spaces can mis-parse).
Prefer executing via an argv array (e.g. cmd=(genai-perf profile ...) and "${cmd[@]}") and append extra args as array elements rather than string concatenation.
| { | ||
| process_args "$@" | ||
|
|
||
| report_file=$result_dir/$report_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible division by zero
num_filler_prompts is computed as 1 + (kv_cache_token_size / num_filler_tokens). If kv_cache_token_size extraction fails and stays 0, or if num_filler_tokens is 0/empty, this can divide by zero or silently produce a bad value that breaks later calls.
Guard the arithmetic (validate both values are positive integers) and fail fast with a clear error when KV cache token size can’t be computed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cloudai/workloads/ai_dynamo/ai_dynamo.sh (3)
535-548:⚠️ Potential issue | 🔴 CriticalBug: Substring-based node matching can produce false positives.
_is_frontend_node,_is_decode_node, and_is_prefill_nodeall use glob*"$name"*against a comma-separated list. A node namednode1will falsely match a list containingnode10.Notably,
_populate_nodelist(line 210) already handles this correctly with,${exclude_nodelist},/,$node,fencing. Apply the same pattern here.🐛 Proposed fix
_is_decode_node() { local name="$(_current_node_name)" - [[ "${decode_config["node-list"]}" == *"$name"* ]] + [[ ",${decode_config["node-list"]}," == *",$name,"* ]] } _is_prefill_node() { local name="$(_current_node_name)" - [[ "${prefill_config["node-list"]}" == *"$name"* ]] + [[ ",${prefill_config["node-list"]}," == *",$name,"* ]] }For
_is_frontend_node, iffrontend-nodeis always a single hostname (not a list), the current pattern is less dangerous but still risky:_is_frontend_node() { local name="$(_current_node_name)" - [[ "${dynamo_args["frontend-node"]}" == *"$name"* ]] + [[ "${dynamo_args["frontend-node"]}" == "$name" ]] }
83-93:⚠️ Potential issue | 🟠 Major
_csv_index_ofuses substring matching, risking false positives on similar hostnames.Line 88's
*"$name"*check meansnode1matchesnode10. This feeds into rank assignment in_apply_sglang_section_args, so a wrong rank could cause multi-node sglang workers to misconfigure.Consider exact matching:
♻️ Proposed fix
for i in "${!arr[@]}"; do - if [[ "${arr[$i]}" == "$name" || "${arr[$i]}" == *"$name"* || "$name" == *"${arr[$i]}"* ]]; then + if [[ "${arr[$i]}" == "$name" ]]; then echo "$i"; return 0 fi doneIf fuzzy matching is intentional (e.g., short vs FQDN hostnames), use a more targeted approach like stripping the domain suffix rather than arbitrary substring containment.
1057-1081:⚠️ Potential issue | 🟠 Major
setup_lmcacheandsetup_kvbmonly run on the frontend node — non-frontend workers won't have the environment set up.Lines 1060-1061 are inside the
if _is_frontend_nodeblock, so decode/prefill nodes on other machines never callsetup_kvbm(which exportsDYN_KVBM_DISK_CACHE_DIRand prepares cache) orsetup_lmcache(which installs the package and writes the config). While the parent shell exports these variables, SLURM-launched worker processes on remote nodes won't inherit them.A
node-setup-cmdmechanism exists (line 45) to run setup on all nodes, but it requires explicit user configuration and isn't automatically invoked. For multi-node deployments with kvbm or lmcache connectors, either move the connector setup outside the frontend conditional block, or document that users must configurenode-setup-cmdto include the necessary setup steps.
🤖 Fix all issues with AI agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh`:
- Around line 839-843: The readiness loop currently bypasses prefill readiness
by hardcoding want_prefill=0 and have_prefill=0; restore the real checks by
calling _expected_ready_prefill for want_prefill and _count_initialized_prefill
for have_prefill (instead of the commented-out calls) so the loop waits for
prefill workers to be ready before continuing, or if this bypass is intentional
add a clear inline comment on why prefill readiness is skipped (referencing
want_prefill, have_prefill, _expected_ready_prefill, _count_initialized_prefill
and the prefill worker launch around the prefill launch site).
- Around line 971-987: The log_gpu_utilization function currently blocks forever
on non-frontend nodes because it calls wait_for_dynamo_frontend which relies on
cluster-wide worker counts
(_count_initialized_decode/_count_initialized_prefill) that only read local
files; remove the wait_for_dynamo_frontend call from log_gpu_utilization and
either (a) drop the cluster-wide wait entirely so GPU monitoring starts
immediately for the worker, or (b) replace it with a simple local readiness
check (e.g., verify the local worker init marker the other functions use) so the
function no longer spins waiting for other nodes; update references in
log_gpu_utilization and keep wait_for_dynamo_frontend for codepaths that truly
need cluster-wide synchronization.
- Around line 775-781: The worker command builds duplicate --extra-args because
array_to_args processes all keys including "--extra-args" and then the script
appends ${decode_args["--extra-args"]} again; update either array_to_args to
skip the "--extra-args" key or remove the explicit
${decode_args["--extra-args"]} append in the command construction where
CUDA_VISIBLE_DEVICES... ${decode_config["cmd"]} $(array_to_args decode_args)
${decode_args["--extra-args"]} > $log_file 2>&1 &, and apply the same fix to the
launch_prefill command block so --extra-args appears only once.
- Around line 224-254: The _set_nodelists function can hit bash arithmetic
errors when decode_config["num-nodes"] or prefill_config["num-nodes"] are empty;
ensure both have safe defaults before any numeric comparison: set
decode_config["num-nodes"] and prefill_config["num-nodes"] to 0 when unset (e.g.
use parameter expansion defaults or explicitly assign) and update the
comparisons that use prefill_num_nodes and decode_nodelist_count (the checks
that call _csv_len and compare to decode_config["num-nodes"] /
prefill_num_nodes) to use the :-0 guard so arithmetic comparisons never receive
an empty string.
| _set_nodelists() | ||
| { | ||
| if [[ -z "${DYNAMO_NODELIST:-}" ]]; then | ||
| log "ERROR: DYNAMO_NODELIST is not set" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ -z "${decode_config["node-list"]}" ]]; then | ||
| decode_config["node-list"]=$(_populate_nodelist "${decode_config["num-nodes"]}" "") | ||
| fi | ||
|
|
||
| if [[ -z "${prefill_config["node-list"]}" ]]; then | ||
| prefill_config["node-list"]=$(_populate_nodelist "${prefill_config["num-nodes"]}" "${decode_config["node-list"]}") | ||
| fi | ||
|
|
||
| # Prefill nodelist should match prefill node count (skip validation if num-nodes is 0) | ||
| local prefill_num_nodes="${prefill_config["num-nodes"]:-0}" | ||
| if [[ "$prefill_num_nodes" -gt 0 ]]; then | ||
| local prefill_nodelist_count=$(_csv_len "${prefill_config["node-list"]}") | ||
| if [[ "${prefill_nodelist_count}" -ne "${prefill_num_nodes}" ]]; then | ||
| log "ERROR: number of nodes in prefill nodelist (${prefill_nodelist_count}) does not match prefill node count (${prefill_num_nodes})" | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| local decode_nodelist_count=$(_csv_len "${decode_config["node-list"]}") | ||
| if [[ "${decode_nodelist_count}" -ne "${decode_config["num-nodes"]}" ]]; then | ||
| log "ERROR: number of nodes in decode nodelist (${decode_nodelist_count}) does not match decode node count (${decode_config["num-nodes"]})" | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing defaults for decode_config["num-nodes"] and prefill_config["num-nodes"] will crash _set_nodelists.
If --decode-num-nodes is never passed, decode_config["num-nodes"] is empty. Line 250 then runs [[ "" -ne "" ]], which is a bash arithmetic error. Same risk for prefill_config["num-nodes"] on line 241 (mitigated by the :-0 default there, but decode has no such guard).
Add defaults in the initialization block or guard the comparison.
🐛 Proposed fix — add defaults
declare -A prefill_config
+prefill_config["num-nodes"]=0
declare -A prefill_args
declare -A decode_config
+decode_config["num-nodes"]=0
declare -A decode_args🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 242-242: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 249-249: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 224 - 254, The
_set_nodelists function can hit bash arithmetic errors when
decode_config["num-nodes"] or prefill_config["num-nodes"] are empty; ensure both
have safe defaults before any numeric comparison: set decode_config["num-nodes"]
and prefill_config["num-nodes"] to 0 when unset (e.g. use parameter expansion
defaults or explicitly assign) and update the comparisons that use
prefill_num_nodes and decode_nodelist_count (the checks that call _csv_len and
compare to decode_config["num-nodes"] / prefill_num_nodes) to use the :-0 guard
so arithmetic comparisons never receive an empty string.
| log "Decode cmd: ${decode_config["cmd"]} $(array_to_args decode_args) ${decode_args["--extra-args"]}" | ||
| CUDA_VISIBLE_DEVICES=$gpu_list \ | ||
| VLLM_NIXL_SIDE_CHANNEL_PORT=$nixl_port \ | ||
| DYN_VLLM_KV_EVENT_PORT=$kv_event_port \ | ||
| ${dynamo_args["decode-cmd"]} \ | ||
| ${decode_config["cmd"]} \ | ||
| $(array_to_args decode_args) ${decode_args["--extra-args"]} > $log_file 2>&1 & | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--extra-args value is emitted twice in the worker command line.
array_to_args decode_args iterates all keys including --extra-args, producing --extra-args <value> in its output. Then ${decode_args["--extra-args"]} appends the raw value again. The same issue applies to launch_prefill (lines 818-823).
Either skip --extra-args inside array_to_args, or remove the separate append.
🐛 Proposed fix — skip the key in `array_to_args`
function array_to_args()
{
local -n arr=$1
local result=""
for key in "${!arr[@]}"; do
+ # Skip --extra-args; appended separately by callers
+ [[ "$key" == "--extra-args" ]] && continue
shopt -s nocasematch
val=$(replace_placeholders "${arr[$key]}")🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 780-780: Quote this to prevent word splitting.
(SC2046)
[info] 780-780: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 780-780: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 775 - 781, The
worker command builds duplicate --extra-args because array_to_args processes all
keys including "--extra-args" and then the script appends
${decode_args["--extra-args"]} again; update either array_to_args to skip the
"--extra-args" key or remove the explicit ${decode_args["--extra-args"]} append
in the command construction where CUDA_VISIBLE_DEVICES...
${decode_config["cmd"]} $(array_to_args decode_args)
${decode_args["--extra-args"]} > $log_file 2>&1 &, and apply the same fix to the
launch_prefill command block so --extra-args appears only once.
| local want_prefill=0 #$(_expected_ready_prefill) | ||
| local want_decode=$(_expected_ready_decode) | ||
|
|
||
| while :; do | ||
| local have_prefill=$(_count_initialized_prefill) | ||
| local have_prefill=0 #$(_count_initialized_prefill) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefill worker readiness is bypassed (hardcoded to 0).
Lines 839 and 843 hardcode want_prefill=0 and have_prefill=0 with the real calls commented out. If prefill workers are launched (line 1080), they won't be waited on before the frontend is declared "ready" and workloads begin.
If intentional, add a comment explaining why. If not, restore the calls.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 840-840: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 839 - 843, The
readiness loop currently bypasses prefill readiness by hardcoding want_prefill=0
and have_prefill=0; restore the real checks by calling _expected_ready_prefill
for want_prefill and _count_initialized_prefill for have_prefill (instead of the
commented-out calls) so the loop waits for prefill workers to be ready before
continuing, or if this bypass is intentional add a clear inline comment on why
prefill readiness is skipped (referencing want_prefill, have_prefill,
_expected_ready_prefill, _count_initialized_prefill and the prefill worker
launch around the prefill launch site).
| function log_gpu_utilization() | ||
| { | ||
| # Check if nvidia-smi is available | ||
| if ! command -v nvidia-smi &> /dev/null; then | ||
| log "Error: nvidia-smi not found" | ||
| return | ||
| fi | ||
|
|
||
| wait_for_dynamo_frontend | ||
| log "Starting GPU utilization monitoring" | ||
|
|
||
| nvidia-smi \ | ||
| --query-gpu=timestamp,name,pci.bus_id,pstate,pcie.link.gen.max,pcie.link.gen.current,temperature.gpu,utilization.gpu,utilization.memory,memory.total,memory.free,memory.used \ | ||
| --format=csv \ | ||
| -l 5 \ | ||
| -f ${RESULTS_DIR}/gpu_utilization-${SLURM_NODEID}.csv | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log_gpu_utilization will never start monitoring on non-frontend nodes.
wait_for_dynamo_frontend (line 979) expects cluster-wide worker counts but only sees local log files via _count_initialized_decode/_count_initialized_prefill. On a non-frontend node, the local worker count will never reach the cluster-wide threshold, so the function spins in the wait loop indefinitely and nvidia-smi monitoring never starts.
Consider either removing the wait_for_dynamo_frontend call from this function (the workers are already running when GPU monitoring matters), or using a simpler local readiness check.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 986-986: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 986-986: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
In `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 971 - 987, The
log_gpu_utilization function currently blocks forever on non-frontend nodes
because it calls wait_for_dynamo_frontend which relies on cluster-wide worker
counts (_count_initialized_decode/_count_initialized_prefill) that only read
local files; remove the wait_for_dynamo_frontend call from log_gpu_utilization
and either (a) drop the cluster-wide wait entirely so GPU monitoring starts
immediately for the worker, or (b) replace it with a simple local readiness
check (e.g., verify the local worker init marker the other functions use) so the
function no longer spins waiting for other nodes; update references in
log_gpu_utilization and keep wait_for_dynamo_frontend for codepaths that truly
need cluster-wide synchronization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24 files reviewed, 9 comments
| # Combine aiperf_args (key-value pairs) and extra_args | ||
| cmdline_args="" | ||
| for key in "${!aiperf_args[@]}"; do | ||
| local val="${aiperf_args[$key]}" | ||
| # Quote values that contain spaces so eval doesn't split them | ||
| if [[ "$val" == *" "* ]]; then | ||
| val="${val//\"/\\\"}" # Escape existing quotes | ||
| cmdline_args+="$key \"${val}\" " | ||
| else | ||
| cmdline_args+="$key ${val} " | ||
| fi | ||
| done | ||
| cmdline_args+="$extra_args" | ||
|
|
||
| # Build the full command with model and url | ||
| full_cmd="$cmd $cmdline_args" | ||
|
|
||
| # launch aiperf | ||
| log "Launching aiperf with args: $full_cmd" | ||
|
|
||
| eval "$full_cmd" | ||
|
|
||
| log "Done with aiperf run" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command injection via eval
aiperf.sh builds full_cmd from cmd, parsed --* args, and extra_args, then executes it with eval "$full_cmd" (line ~165). This is unsafe and will also mis-handle quoting/escaping for many legitimate values (e.g., JSON, paths with spaces), since eval re-parses the string as shell code.
This is the same issue already noted for genai_perf.sh; it also affects this new script and should be fixed the same way (execute via an argv array / no eval).
| } | ||
|
|
||
| client = OpenAI(base_url=args.url, api_key="dummy-key-for-local-server") | ||
|
|
||
| if args.compute_kv_cache_token_size: | ||
| log(f"Computing KV cache token size...") | ||
| kv_cache_token_size = compute_kv_cache_token_size(args, client); | ||
| log(f"KV cache token size: {kv_cache_token_size}") | ||
| with Path(args.out).open("a", encoding="utf-8") as f: | ||
| f.write(f"KV cache token size: {kv_cache_token_size}\n") | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crash when --out unset
main() writes results using Path(args.out).open(...) in multiple branches (e.g., --compute_kv_cache_token_size and --dump_csv_header), but --out is not required in parse_args() (line ~148) and defaults to None. If a user runs with --dump_csv_header (or --compute_kv_cache_token_size) without --out, this will raise a TypeError when constructing Path(None).
Make --out required for modes that write output (or provide a default).
| artifacts_dir="${result_dir}/lmbench_artifacts" | ||
| mkdir -p "$artifacts_dir" | ||
|
|
||
| for qps in ${all_qps//,/ }; do | ||
| log "Launching lmbench with args: $cmd $cmdline_args --qps $qps --output $output_file" | ||
| output_file="${artifacts_dir}/lmbench_bench_output_${qps}.csv" | ||
| report_file="${result_dir}/${report_name//.csv/_${qps}.csv}" | ||
| eval "$cmd $cmdline_args --qps $qps --output $output_file" > "${artifacts_dir}/lmbench_bench_output_${qps}.log" 2>&1 | ||
| python3 ${install_dir}/${calc_percentile_csv_script} --input $output_file --output ${report_file} | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output_file used before set
Inside the QPS loop, the log line uses --output $output_file before output_file is assigned (line ~111-113). This means logs will show an empty/previous value and makes debugging harder; it also indicates the command composition order is wrong.
Move the output_file=... assignment before it’s referenced in the log / command string.
| local workload_name="${workload_config_ref["--name"]}" | ||
| local script="${workload_config_ref["--script"]}" | ||
| local expanded_config=$(array_to_args "$workload_config_name") | ||
| local expanded_arguments=$(array_to_args "$workload_args_name") | ||
|
|
||
| log "Launching $workload_name with cmd: ${INSTALL_DIR}/$script $expanded_config -- $expanded_arguments" | ||
|
|
||
| # Use eval to properly handle values with spaces in expanded_config | ||
| bash ${INSTALL_DIR}/$script \ | ||
| --install_dir "$INSTALL_DIR" \ | ||
| --result_dir "$RESULTS_DIR" \ | ||
| --model "${dynamo_args["model"]}" \ | ||
| --url "http://${dynamo_args["frontend-node"]}" \ | ||
| --port "${dynamo_args["port"]}" \ | ||
| --endpoint "${dynamo_args["endpoint"]}" \ | ||
| --gpus_per_node "$(_gpus_per_node)" \ | ||
| --connector "${dynamo_args["connector"]}" \ | ||
| --kvbm_metrics_port "${DYN_KVBM_METRICS_PORT:-6880}" \ | ||
| --decode-nodes "${decode_config["node-list"]}" \ | ||
| $expanded_config \ | ||
| -- $expanded_arguments > ${RESULTS_DIR}/$workload_name.log 2>&1 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg injection via unquoted expansions
launch_workload() expands $expanded_config and $expanded_arguments directly into the bash ${INSTALL_DIR}/$script ... invocation (lines ~1020-1032) without quoting. If any values contain spaces, quotes, or shell metacharacters, they will be split or interpreted as separate flags/commands, breaking the run and potentially allowing unintended argument injection.
This is especially likely now that TOML can include JSON-like strings (e.g., extra-inputs). Consider building an argv array and appending elements, rather than concatenating/expanding strings.
|
|
||
| # Calculate total G1 memory across all GPUs | ||
| local g1_memory_total_gib=$(awk "BEGIN {printf \"%.2f\", $g1_memory_per_gpu_gib * $tp_size}") | ||
| log "G1 (GPU) total memory across $tp_size GPUs: $g1_memory_total_gib GiB" | ||
|
|
||
| # Calculate bytes per token = (total_G1_GiB * 1024^3) / G1_tokens | ||
| # Using awk to handle the initial float-to-int conversion from g1_memory_per_gpu_gib | ||
| bytes_per_token=$(awk "BEGIN {printf \"%d\", ($g1_memory_total_gib * 1024 * 1024 * 1024) / $g1_token_size}") | ||
| log "Calculated bytes per token: $bytes_per_token" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Division by zero in KV sizing
bytes_per_token is computed as (...)/ $g1_token_size (line ~249) after parsing g1_token_size from logs. If the log parse yields 0 (or empty) for any reason, this will trigger division by zero / awk error and cascade into invalid cache size calculations.
This needs a guard that g1_token_size is a positive integer before doing the division.
| report_file=$result_dir/$report_name | ||
|
|
||
| compute_kv_cache_token_size | ||
| local num_filler_prompts=$(( 1 + (kv_cache_token_size / num_filler_tokens) )) | ||
|
|
||
| log "Dumping CSV header" | ||
| python3 ${install_dir}/openai_chat_client.py --dump_csv_header --out $report_file | ||
|
|
||
| log "Launching custom workload with ISLs: $all_isl" | ||
| for isl in $(echo $all_isl | tr ',' '\n'); do | ||
| log "Launching custom workload with ISL: $isl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can divide by zero
num_filler_prompts is computed as 1 + (kv_cache_token_size / num_filler_tokens) (line ~326). If num_filler_tokens is 0/empty, this will raise a shell arithmetic error; if kv_cache_token_size extraction fails and remains 0, this silently produces a misleading value.
Validate both kv_cache_token_size and num_filler_tokens are positive integers before this computation.
| def constraint_check(self, tr: TestRun) -> bool: | ||
| prefill_worker = tr.test.cmd_args.dynamo.prefill_worker | ||
| decode_worker = tr.test.cmd_args.dynamo.decode_worker | ||
|
|
||
| prefill_tp = prefill_worker.args.tensor_parallel_size if prefill_worker else 1 | ||
| decode_tp = decode_worker.args.tensor_parallel_size if decode_worker else 1 | ||
| prefill_pp = prefill_worker.args.pipeline_parallel_size if prefill_worker else 1 | ||
| decode_pp = decode_worker.args.pipeline_parallel_size if decode_worker else 1 | ||
| prefill_nodes = prefill_worker.num_nodes if prefill_worker else 0 | ||
| decode_nodes = decode_worker.num_nodes if decode_worker else 1 | ||
|
|
||
| if self.constraints.prefill_tp_le_decode_tp and prefill_tp > decode_tp: | ||
| logging.info("constraint_check failed for: prefill_tp_le_decode_tp") | ||
| return False | ||
| logging.info("constraint_check passed for: prefill_tp_le_decode_tp") | ||
|
|
||
| gpus_per_node = tr.test.cmd_args.gpus_per_node | ||
| if self.constraints.tp_times_pp_le_gpus_per_node and ( | ||
| prefill_tp * prefill_pp > gpus_per_node or decode_tp * decode_pp > gpus_per_node | ||
| ): | ||
| logging.info("constraint_check failed for: tp_times_pp_le_gpus_per_node") | ||
| return False | ||
| logging.info("constraint_check passed for: tp_times_pp_le_gpus_per_node") | ||
|
|
||
| num_nodes = tr.test.cmd_args.num_nodes | ||
| nodes_check = self.constraints.prefill_decode_nodes_le_total_nodes | ||
| if nodes_check and prefill_nodes + decode_nodes > num_nodes: | ||
| logging.info("constraint_check failed for: prefill_decode_nodes_le_total_nodes") | ||
| return False | ||
| logging.info("constraint_check passed for: prefill_decode_nodes_le_total_nodes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constraint check type crash
constraint_check() multiplies/compares prefill_tp, prefill_pp, decode_tp, decode_pp, but these fields are declared as int | list[int] | None and can be None (or a list) when unset in TOML. That makes expressions like prefill_tp * prefill_pp and prefill_tp > decode_tp raise TypeError at runtime for common configs.
Normalize these values to integers (with defaults) before doing arithmetic/comparisons.
Additional Comments (2)
This should be
This should branch on the actual type (int vs list/str) before comparing/using it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24 files reviewed, 6 comments
|
|
||
|
|
||
| def compute_kv_cache_token_size(args: argparse.Namespace, client: OpenAI) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken min filler prompts
In compute_kv_cache_token_size(), minFillerPrompts is initialized as min(1, args.min_filler_prompts), which will always be <= 1 (and becomes 1 for any normal --min_filler_prompts >= 1). This breaks the intended binary-search bounds and can’t honor a user-provided minimum > 1. It should likely be max(1, args.min_filler_prompts) (or just args.min_filler_prompts if you want to allow 0).
| except Exception: | ||
| return float("nan") | ||
|
|
||
|
|
||
| def main() -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crashes when --input omitted
main() uses input_path = args.input and then builds output_path = args.output or f"{input_path}_summary.csv", but --input isn’t required. If the user forgets --input, this becomes None_summary.csv and then open(input_path, ...) raises TypeError. Make --input required (or handle None with a clear error message).
|
|
||
| log("Sending an initial canary query with 1000 tokens...") | ||
| canary_chat = Chat(args.model, args.isl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binary-search bounds wrong
compute_kv_cache_token_size() sets minFillerPrompts = min(1, args.min_filler_prompts), which collapses the lower bound to 0/1 and can’t honor --min_filler_prompts > 1. This makes the “min filler prompts” CLI ineffective and changes the search behavior. Likely meant max(1, args.min_filler_prompts) (or just args.min_filler_prompts if 0 is valid).
|
|
||
| rows: List[Dict[str, Any]] = [] | ||
| with open(input_path, newline="") as f: | ||
| reader = csv.DictReader(f) | ||
| for r in reader: | ||
| rows.append(r) | ||
|
|
||
| # Build summaries | ||
| summaries: List[Dict[str, Any]] = [] | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--input None causes crash
main() doesn’t require --input/-i, but immediately does output_path = args.output or f"{input_path}_summary.csv" and then open(input_path, ...). If --input is omitted, input_path is None and open(None, ...) raises TypeError. Make --input required or explicitly error out when it’s missing.
| logging.info(f"Workload {workload} not found in workload map") | ||
| result = False | ||
| continue | ||
| report_name = workload_map[workload].report_name | ||
| assert report_name is not None | ||
| workload_csv_file = output_path / report_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constraint_check TypeError
constraint_check() pulls tensor_parallel_size / pipeline_parallel_size from WorkerConfig.args, but those fields are declared as int | list[int] | None. When unset they’ll be None, and expressions like prefill_tp > decode_tp or prefill_tp * prefill_pp will raise TypeError at runtime. This can happen with common configs that omit TP/PP. Normalize these values to ints (with defaults like 1) before comparisons/multiplication.
| result = list[str]() | ||
|
|
||
| mounts = [ | ||
| f"{dynamo_repo_path}:{dynamo_repo_path}", | ||
| f"{self.system.hf_home_path.absolute()}:{td.cmd_args.huggingface_home_container_path}", | ||
| f"{td.script.installed_path.absolute()!s}:{td.script.installed_path.absolute()!s}", | ||
| ] | ||
| logging.info(f"hf_home_path: {td.cmd_args.hf_home_path}") | ||
| logging.info(f"storage_cache_dir: {td.cmd_args.storage_cache_dir}") | ||
| if td.cmd_args.hf_home_path: | ||
| result.append(f"{td.cmd_args.hf_home_path}:{td.cmd_args.hf_home_path}") | ||
|
|
||
| if td.cmd_args.dynamo.backend == "sglang": | ||
| deepep_path = ( | ||
| dynamo_repo_path / "components/backends/sglang/configs/deepseek_r1/wideep/deepep.json" | ||
| ).absolute() | ||
| sgl_http_server_path = ( | ||
| dynamo_repo_path / "components/backends/sglang/src/dynamo/sglang/utils/sgl_http_server.py" | ||
| ).absolute() | ||
| mounts.extend( | ||
| [ | ||
| f"{deepep_path!s}:{deepep_path!s}", | ||
| f"{sgl_http_server_path!s}:{sgl_http_server_path!s}", | ||
| ] | ||
| ) | ||
| return mounts | ||
| if td.cmd_args.storage_cache_dir: | ||
| result.append(f"{td.cmd_args.storage_cache_dir}:{td.cmd_args.storage_cache_dir}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing required container mounts
_container_mounts() used to always mount the installed Dynamo repo + workload script into the container. After this change it returns only optional hf_home_path / storage_cache_dir mounts. But the generated srun command now runs bash {self.container_install_path}/{td.script.src.name} and passes --dynamo-repo {td.dynamo_repo.container_mount}; without mounting/installing those paths, the container won’t find the script/repo and the job will fail to start. Restore the required mounts (install dir/results dir and the dynamo repo mount) so the container runtime can access them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24 files reviewed, 3 comments
| # Build the full command with model and url | ||
| full_cmd="$cmd $cmdline_args" | ||
|
|
||
| # launch aiperf | ||
| log "Launching aiperf with args: $full_cmd" | ||
|
|
||
| eval "$full_cmd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsafe eval execution
full_cmd is assembled from config/CLI-derived values (cmd, aiperf_args, extra_args) and then executed via eval. This will mis-handle quoting for legitimate values and will execute shell metacharacters if they appear in config (e.g., $(...), backticks, ;, pipes), which can alter the invoked command. Please switch to executing an argv array (no eval).
Also appears in src/cloudai/workloads/ai_dynamo/genai_perf.sh:149-155 and src/cloudai/workloads/ai_dynamo/lmbench.sh:105-115.
| logging.debug(f"Copying wrapper script {wrapper_script_path} to pod {frontend_pod}") | ||
| cp_wrapper_cmd = f"kubectl cp {wrapper_script_path} {self.default_namespace}/{frontend_pod}:{pod_wrapper_path}" | ||
| subprocess.run(cp_wrapper_cmd, shell=True, capture_output=True, text=True, check=True) | ||
|
|
||
| logging.debug(f"Copying calc_percentile_csv script {calc_csv_script_path} to pod {frontend_pod}") | ||
| cp_calc_cmd = f"kubectl cp {calc_csv_script_path} {self.default_namespace}/{frontend_pod}:{pod_calc_csv_path}" | ||
| subprocess.run(cp_calc_cmd, shell=True, capture_output=True, text=True, check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell=True kubectl cp
kubectl cp is built as a string and run with shell=True. If wrapper_script_path / calc_csv_script_path (or pod/namespace) contain repeated or shell metacharacters, this can break copying or execute unintended shell tokens. Since subprocess.run supports argv lists, this should be invoked without a shell.
Note: this pattern also occurs later for cp_logs_cmd in kubernetes_system.py:381-391.
| for qps in ${all_qps//,/ }; do | ||
| log "Launching lmbench with args: $cmd $cmdline_args --qps $qps --output $output_file" | ||
| output_file="${artifacts_dir}/lmbench_bench_output_${qps}.csv" | ||
| report_file="${result_dir}/${report_name//.csv/_${qps}.csv}" | ||
| eval "$cmd $cmdline_args --qps $qps --output $output_file" > "${artifacts_dir}/lmbench_bench_output_${qps}.log" 2>&1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses output_file before set
Inside the QPS loop, the log line includes --output $output_file before output_file is assigned (it’s set on the next line). This will log an empty/previous value and makes the loop’s command composition misleading when debugging. Move the output_file=... assignment before it’s referenced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 files reviewed, 6 comments
| # run LMBenchmark, adjust the model name if you are using a different model | ||
| # for detail how to config and run LMBenchmark: https://github.com/LMCache/LMBenchmark/tree/main/synthetic-multi-round-qa | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output_file used before set
output_file is referenced in the log message before it’s assigned (inside the QPS loop), so the log prints an empty/previous value and the command line is misleading during debugging. Move the output_file=... assignment above the log line (and ideally build/execute the command after all variables are set).
| #export NUM_USERS_WARMUP="20" | ||
| #export NUM_USERS="15" | ||
| #export NUM_ROUNDS="20" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eval executes user input
This executes eval "$cmd $cmdline_args ..." where cmd, cmdline_args, and extra_args come from CLI/config. Any quotes/spaces/metacharacters will be re-parsed as shell code, which can break valid arguments (e.g., JSON) and enables command injection when values are attacker-controlled. Prefer executing via an argv array (no eval) and pass --qps/--output as separate elements.
| logging.debug(f"Copying calc_percentile_csv script {calc_csv_script_path} to pod {frontend_pod}") | ||
| cp_calc_cmd = f"kubectl cp {calc_csv_script_path} {self.default_namespace}/{frontend_pod}:{pod_calc_csv_path}" | ||
| subprocess.run(cp_calc_cmd, shell=True, capture_output=True, text=True, check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell=True kubectl cp
kubectl cp is constructed as a single string and executed with shell=True (wrapper + calc script copies). If any of the interpolated values contain spaces or shell metacharacters (paths, namespace, pod name), this can mis-copy or execute unintended shell tokens. Use subprocess.run(["kubectl","cp",...], check=True, ...) (no shell) or properly quote/escape all components.
| default=NUM_FILLER_TOKENS, | ||
| help="Number of filler tokens.", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Min filler bound broken
In compute_kv_cache_token_size(), minFillerPrompts = min(1, args.min_filler_prompts) collapses the lower bound to <= 1, so --min_filler_prompts can never be honored when set to values > 1. This breaks the intended binary-search bounds; it likely should be max(1, args.min_filler_prompts) (or just args.min_filler_prompts if 0 is valid).
| numFillerPrompts = (maxFillerPrompts + minFillerPrompts) // 2 | ||
| log(f"Trying {numFillerPrompts} filler prompts with {args.num_filler_tokens} tokens each...") | ||
| SendFillerQueries(args, client, numFillerPrompts) | ||
| log(f"Sending canary query after {numFillerPrompts} filler prompts...") | ||
| canary_stats = canary_chat.stream(client, 1) | ||
| log(f"Canary query: {canary_stats.ttft_seconds:.3f}s with {canary_stats.cached_tokens} cached tokens") | ||
| if canary_stats.cached_tokens < 500: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crash when --out missing
--out is optional in parse_args(), but main() unconditionally does Path(args.out).open(...) in --dump_csv_header and --compute_kv_cache_token_size modes. Running either flag without --out raises TypeError (Path(None)). Make --out required for write modes or provide a default output destination.
|
|
||
| input_path = args.input | ||
| output_path = args.output or f"{input_path}_summary.csv" | ||
|
|
||
| rows: List[Dict[str, Any]] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--input can be None
--input/-i isn’t required, but main() immediately builds output_path = ... f"{input_path}_summary.csv" and then open(input_path, ...). If --input is omitted, this will crash with TypeError. Make --input required or explicitly error out when it’s missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
12 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
24 files reviewed, 2 comments
| function split_csv_sections() | ||
| { | ||
| # Split a CSV file into separate files based on blank-line delimiters. | ||
| # Usage: split_csv_sections <input_file> <output_prefix> | ||
| # Produces: <output_prefix>.1.csv, <output_prefix>.2.csv, <output_prefix>.3.csv, ... | ||
| local input_file="$1" | ||
| local output_prefix="$2" | ||
|
|
||
|
|
||
| echo "$section" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused broken helper
split_csv_sections() is defined with unused params and references echo "$section" where section is never set. Since it’s not called, this is dead code, but if invoked later it will behave incorrectly. Consider removing the stub entirely or implementing it and using it consistently.
| done < "$profile_path" | ||
|
|
||
| log "Split aiperf CSV into $num_sections section(s)" | ||
|
|
||
| # Section 1: per-request percentile metrics → main report | ||
| if [[ -f "${output_prefix}.1.csv" ]]; then | ||
| mv "${output_prefix}.1.csv" "$report_file" | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing report file
The new section-splitting logic only creates/moves the main report_file if section 1 is produced. For empty/blank-only aiperf CSVs (or inputs that don’t match the expected section delimiters), this leaves no report_file at the expected path, and downstream report parsing will fail. Consider a fallback path that always writes a report_file (even if empty) or explicitly errors out with a clear message when parsing yields no section 1.
Summary
Several updates to bring it up to speed with the latest AI-Dynamo code base and use cases.
Test Plan